Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not create default wired connections for bond ports #5703

Merged

Conversation

rvykydal
Copy link
Contributor

Resolves: RHEL-38451

@rvykydal
Copy link
Contributor Author

/kickstar-test --testtype network

@rvykydal
Copy link
Contributor Author

/kickstart-test --testtype network

@rvykydal
Copy link
Contributor Author

/kickstart-test --testtype network

@rvykydal
Copy link
Contributor Author

Kickstart tests update: rhinstaller/kickstart-tests#1221

@rvykydal
Copy link
Contributor Author

Related kickstart-tests update: rhinstaller/kickstart-tests#1221

@rvykydal
Copy link
Contributor Author

Related kickstart-tests update: rhinstaller/kickstart-tests#1221

I've checked locally that the failing tests pass after update in rhinstaller/kickstart-tests#1221
(also created an issue to be able to test it right here: https://issues.redhat.com/browse/INSTALLER-3981
Same for the rhel version: #5704 (In rhel there is an importatnt difference in networking: it does not create default NM autoconnections in installer environment)

@rvykydal
Copy link
Contributor Author

/kickstart-test --testtype network

Copy link
Contributor

@KKoukiou KKoukiou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good and thanks for the test updates.

Copy link
Member

@jkonecny12 jkonecny12 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me otherwise. Thanks!

Comment on lines +272 to +280
if not dumped_con:
dumped_con = self._select_persistent_connection_for_device(device, available_cons)

if not dumped_con and len(initramfs_cons) == 1:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can put the second if under the first one and remove the not dumped_con part of the condition.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not following, could you please rather write it down. The intended logic here is to assign the dumped_con in steps in order of priority, like if we find persistent connection we are done here.

Comment on lines 308 to 318
(NM.SETTING_CONNECTION_SETTING_NAME,
NM.SETTING_CONNECTION_AUTOCONNECT,
True),
# Update cloned generic connection from initramfs
(NM.SETTING_CONNECTION_SETTING_NAME,
NM.SETTING_CONNECTION_MULTI_CONNECT,
0),
# Update cloned generic connection from initramfs
(NM.SETTING_CONNECTION_SETTING_NAME,
NM.SETTING_CONNECTION_WAIT_DEVICE_TIMEOUT,
-1)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The indentation seems to be weird here. Is it correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the catch, updated.

@rvykydal rvykydal marked this pull request as ready for review June 19, 2024 14:21
@rvykydal rvykydal force-pushed the bond-no-default-wired-connection branch from cefa41d to dd6ae13 Compare June 20, 2024 07:06
@rvykydal rvykydal force-pushed the bond-no-default-wired-connection branch from dd6ae13 to 556348e Compare June 20, 2024 07:07
@rvykydal
Copy link
Contributor Author

/kickstart-test --testtype smoke

@rvykydal rvykydal merged commit 0b1248e into rhinstaller:master Jun 20, 2024
14 of 15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 participants