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.

Sign up for GitHub

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

Jump to bottom

Do not create default wired connections for bond ports #5703

Merged
merged 2 commits into from
Jun 20, 2024
Merged

Do not create default wired connections for bond ports #5703

merged 2 commits into from
Jun 20, 2024

Conversation

rvykydal
Copy link
Contributor

Resolves: RHEL-38451

@rvykydal rvykydal temporarily deployed to gh-cockpituous June 11, 2024 14:13 — with GitHub Actions Inactive
@github-actions github-actions bot added the f41 label Jun 11, 2024
@rvykydal
Copy link
Contributor Author

/kickstar-test --testtype network

@rvykydal
Copy link
Contributor Author

/kickstart-test --testtype network

@rvykydal rvykydal force-pushed the bond-no-default-wired-connection branch from 464cf82 to 104ed84 Compare June 16, 2024 17:17
@rvykydal rvykydal temporarily deployed to gh-cockpituous June 16, 2024 17:17 — with GitHub Actions Inactive
@rvykydal rvykydal force-pushed the bond-no-default-wired-connection branch from 104ed84 to 9523983 Compare June 16, 2024 17:17
@rvykydal
Copy link
Contributor Author

/kickstart-test --testtype network

@rvykydal rvykydal force-pushed the bond-no-default-wired-connection branch from 9523983 to 0639de3 Compare June 17, 2024 07:32
@rvykydal rvykydal temporarily deployed to gh-cockpituous June 17, 2024 07:32 — with GitHub Actions Inactive
@rvykydal rvykydal mentioned this pull request Jun 17, 2024
Merged
@rvykydal rvykydal force-pushed the bond-no-default-wired-connection branch from 0639de3 to cefa41d Compare June 17, 2024 07:35
@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

KKoukiou
KKoukiou approved these changes Jun 18, 2024
View reviewed changes
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.

jkonecny12
jkonecny12 approved these changes Jun 19, 2024
View reviewed changes
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!

pyanaconda/modules/network/initialization.py
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.

pyanaconda/modules/network/initialization.py Outdated
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 rvykydal temporarily deployed to gh-cockpituous June 20, 2024 07:07 — with GitHub Actions Inactive
@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
@rvykydal rvykydal mentioned this pull request Jun 23, 2024
Merged
@rvykydal rvykydal mentioned this pull request Jun 28, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@KKoukiou KKoukiou KKoukiou approved these changes

@jkonecny12 jkonecny12 jkonecny12 approved these changes

Assignees
No one assigned
Labels
f41
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rvykydal @jkonecny12 @KKoukiou