Fix filename inheritence

[watologo1]

Description

The check for parent in profile item setter is wrong: profile always has a parent (distro).

It has to be checked whether the parent is (not) of type profile anymore (is_subobject). In this case we know we are at the end of inheritance hierarchy and we have to set filename=""

Behaviour changes

Old: filename might end up in <> string, even the end of inheritance hierarchy was reached. This e.g. could end up in bad dhcpd.conf generation inside a host declaration:
filename <>

New: Make sure a system or profile item always returns the defined filename value or an empty string, but not "<>"

Category

• Bugfix

[watologo1]

Unfortunately I cannot add
tpw56j [email protected]
as reviewer. Would be great if he has a look at this. He worked on related code parts.

[SchoolGuy]

CC @tpw56j

[tpw56j]

The semantics of the Item.parent property has changed, now this property never contains a pointer to an object of another type. Therefore, this change in reality does not change anything.

[tpw56j]

And this code breaks the inheritance of profile.filename.

[tpw56j]

@watologo1 Perhaps the problem is in the template used for dhcp or something else?
Could you please describe the problem in more detail?

[tpw56j]

Sorry, I was wrong - the inheritance logic for filename is really not quite correct. At the moment I see 2 problems:

1. The value of filename depends on the order in which profile properties are deserialized. Neither parent nor is_subobject can be used in filename.setter
2. the inability to explicitly set an empty string for a value in a subprofile if it is explicitly set to a non-empty value in one of the ancestor profiles and this value in the ancestor is not <<inherit>>

It seems to me that the problems arose because the line of inheritance of filename is broken at the base profile.
Therefore, I suggest:

1. add settings.default_filename
2. change
   

   cobbler/cobbler/items/profile.py

   Line 207 in 254b8aa

   self._filename = ""

   
   to
   self._filename = "<<inherit>>"
3. fix filename.setter as it is done for other properties with inheritance from settings

[watologo1]

Sorry, I was wrong - the inheritance logic for filename is really not quite correct. At the moment I see 2 problems:

1. The value of `filename` depends on the order in which profile properties are deserialized. Neither `parent` nor `is_subobject` can be used in `filename.setter`

2. the inability to explicitly set an empty string for a value in a subprofile if it is explicitly set to a non-empty value in one of the ancestor profiles and this value in the ancestor is not `<<inherit>>`

Not sure I get this right...
You say, the condition (at least at this code part), that a profile still has another profile as parent or does not (and then it is the last object in inheritance hierarchy) cannot be detected? It has to somehow?

I agree that the fact that inheritance stops at profile is not perfect. It would be fine to add it to distro as well, in fact it may be helpful, typically every distro should be booted via the same grub binary (which may get overridden for a whole specific distro until a grub bug got fixed or similar as an example scenario).

But I expect enhancing inheritance does not buy much, as handling the update case would make things even worse?

Maybe you still have a clever idea, ortherwise I try to come up with something the next days.

Thanks a lot for your review!

[tpw56j]

@watologo1

Not sure I get this right... You say, the condition (at least at this code part), that a profile still has another profile as parent or does not (and then it is the last object in inheritance hierarchy) cannot be detected? It has to somehow?

I wanted to say that the value of a property of an object when set in a setter should not depend on the values of other properties of the same object. Otherwise, we will get logical inconsistency and unpredictable behavior.

Example: when the cobbler starts, the json file is read, the corresponding object is created, and the values of its properties are set in the Item.from_dict procedure in a loop. Suppose in this json file is_subobject=True (parent != None), and let's say that the value of the filename property in the loop is set before is_subobject (or parent). Then when calculating filename we will use the invalid not yet set value of is_subobject, which at this point will be equal to the default value of is_subobject=False (parent=None).

To eliminate logical contradictions, the Profile.check_if_valid procedure is used, but it cannot fix this particular situation.

I agree that the fact that inheritance stops at profile is not perfect. It would be fine to add it to distro as well, in fact it may be helpful, typically every distro should be booted via the same grub binary (which may get overridden for a whole specific distro until a grub bug got fixed or similar as an example scenario).

In this case, inheriting the filename value from settings is not only useful, but a way to eliminate logical inconsistency.

[SchoolGuy]

@watologo1 Any update on this PR?