Fixes #16935 - Allow a : or . in device ID if there isnt already one set #3942
Conversation
|
There were the following issues with the commit message:
If you don't have a ticket number, please create an issue in Redmine, selecting the appropriate project. More guidelines are available in Coding Standards or on the Foreman wiki. This message was auto-generated by Foreman's prprocessor |
|
There were the following issues with the commit message:
If you don't have a ticket number, please create an issue in Redmine, selecting the appropriate project. More guidelines are available in Coding Standards or on the Foreman wiki. This message was auto-generated by Foreman's prprocessor |
|
looks good to me, could you please add a test to test/models/nics/managed_test.rb? there are already some for the same method |
| @@ -74,9 +74,11 @@ def identifier_change | |||
| old_value = self.identifier_was || '' | |||
There was a problem hiding this comment.
Note there is a comment on line 71 stating that a change to/from virtual/physical is disallowed, but this should be updated to state that you're now permitting a change from physical to virtual (but you're not allowing virtual to physical, for whatever reason.)
There was a problem hiding this comment.
The reason behind this is s390x machines issues dynamic MAC addresses on boot (so I'm told) to configure s390x NIC's via kickstart on the network line/s you need to use --device=<device ID> e.g --device="enccw0.0.1234" (s390x device names have periods in). I ran into this problem where I couldn't add a device ID to a NIC without deleting and adding it again.
I wouldn't say your are changing a device type if you never specified it in the first place?
The good news is to fully support s390x provisioning via kickstart it looks like we just need to make some template changes and a few changes to the host form validation.
There was a problem hiding this comment.
@sean797 the problem is that interface is considered physical if identifier is not set. I tend to agree that until it's set, we can't reliably say what kind of interface it is. For interfaces imported from puppet we always have the identifier from facts so this should be safe.
But based on what you say, the problem is that we consider all interfaces with dots in name as virtual which is not true on s390x. So I wonder if we shouldn't rather consider enccw.* as physical interfaces. I'm not sure if all s390x identifiers start with this prefix though.
There was a problem hiding this comment.
@ares el7 uses names like the ones in systemd/systemd@e0d4a0a recent Fedora has ones like systemd/systemd@0037a66
So in recent Fedora enccw0.0.0600 becomes enc600 (solving our problem). I also don't really like treating it differently if the name is prefixed with something, but I'm not really sure what to suggest.
There was a problem hiding this comment.
To be honest this change doesn't really have anything to do with s390x support, i just happened to run into this problem when provisioning s390x. Like @ares says "I tend to agree that until it's set, we can't reliably say what kind of interface it is.", I think we should allow changing interface type, if it's not already set?
There was a problem hiding this comment.
I can't find a scenario where this would break. Interfaces have empty identifier only when they are built in Foreman and if it causes problems in some cases, user can set identifiers explicitly. So 👍 from me. Test looks good, I'll give it few tests and if everything is alright and no one objects I'll merge.
theforeman-bot
added
Needs re-review
Needs testing
and removed
Waiting on contributor
labels
|
Test added :-) |
|
I'd also add |
|
@lzap are you sure that facter reports interface like this or is it only output of |
|
This was |
|
One thing that appeared during testing is that we do not reflect the change in "Virtual NIC" checkbox. Imagine I create a new interface without identifier leaving "Virtual NIC" unchecked. Then I change its identifier to "eth0:0", I can't change the checkbox since it's disabled for existing records. On the other hand it seems we use this attribute everywhere so if identifier is changed later it should not do any harm. It's just slightly confusing that Bottom line, if others don't mind the conflict described above, I think we could go further and remove the |
|
@ares maybe we should just validate that if "Virtual NIC" checkbox is ticked it has a : or . in the identifier and remove the |
|
@sean797 maybe even further, just remove the validation. Both vlans and aliases can have custom name. If user changes "eth0.5" to "vlan0" it's fine as long as the checkbox remains ticked. Since we keep the checkbox disabled for persisted interfaces, it should be safe. The only place where we use identifier to determine type I'm aware of is |
theforeman-bot
added
Needs re-review
Needs testing
and removed
Waiting on contributor
labels
|
Agreed :-) Updated! |
|
@ares You're right that we're safe because of the checkbox disabled for persisted interfaces. However, I'm not very convinced it should be removed. The reason you added this check in the first place is to avoid a bug where:
Maybe I'm missing something and I definitely have not tried it but it seems to me that's a possible bug that can happen - and users hit it before the validation was introduced |
@dLobatog I don't think this was the reason. Duplicated interfaces usually came from fact uploads which used to skip validations. It's not the case anymore, invalid interface is not saved and errors are logged only. Also users should be able to delete duplicated interfaces now, there were couple of fixes. |
|
Works fine, I'm setting RfM and wait a while to give Daniel and others chance to comment. |
No description provided.