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

redhat: adjust IP bonding to bond0 only #58

Merged
merged 7 commits into from
Sep 13, 2022

Conversation

dustinmiller
Copy link
Contributor

It's possible that adding IPs to bond1 can create a scenario where
networking isn't happy. Lets remove those accordingly

It's possible that adding IPs to bond1 can create a scenario where 
networking isn't happy. Lets remove those accordingly
@mikemrm
Copy link
Contributor

mikemrm commented Aug 11, 2022

we need to add the tests for this too to confirm the fix

It gets replaced by a prior formatting pass
Copy link
Contributor

@nuclearbob nuclearbob left a comment

Choose a reason for hiding this comment

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

bond isn't a variable in the jinja environment, it gets replaced in a formatting pass before the template is rendered. These changes allowed the tests to pass locally for me.

dustinmiller and others added 2 commits August 22, 2022 08:53
…_network-scripts_ifcfg-bondX.j2

Co-authored-by: nuclearbob <6314562+nuclearbob@users.noreply.github.com>
…_network-scripts_ifcfg-bondX.j2

Co-authored-by: nuclearbob <6314562+nuclearbob@users.noreply.github.com>
@dustinmiller dustinmiller marked this pull request as ready for review August 22, 2022 15:53
nuclearbob
nuclearbob previously approved these changes Aug 22, 2022
Copy link
Contributor

@nuclearbob nuclearbob left a comment

Choose a reason for hiding this comment

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

Ship it!

Copy link
Contributor

@mikemrm mikemrm left a comment

Choose a reason for hiding this comment

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

Other than this, and testing changes. looks good

@@ -1,5 +1,6 @@
DEVICE={bond}
NAME={bond}
{{% if "{bond}" == "bond0" %}}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
{{% if "{bond}" == "bond0" %}}
{{% if bond == "bond0" %}}

Copy link
Contributor

Choose a reason for hiding this comment

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

🤦 this is python formatted before it's handled with jinja2, so this is proper

Copy link
Contributor

@mikemrm mikemrm left a comment

Choose a reason for hiding this comment

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

once live tests have been completed, 👍

@nuclearbob
Copy link
Contributor

Testing on actual machines went well except for n3.xlarge.x86, which shows problems on alma_8, rocky_8, and centos_8. alma and rocky install correctly with current osie, so I'm looking into what goes wrong when we use the new one.

@nuclearbob
Copy link
Contributor

alma_8 is now broken with or without this, so let's go ahead and land it, and I'll keep working on alma.

@nuclearbob
Copy link
Contributor

Oh, let's get a review from another team? Maybe @nshalman can recommend someone?

@nuclearbob nuclearbob requested a review from nshalman September 1, 2022 15:21
@nuclearbob
Copy link
Contributor

Do we still want a review from another team on this, or is it okay to go in since it's a fix requested by another team and it's pretty small? Either way, I'd like to keep moving on this since it fixes issues in some of the plans we're enabling.

@dustinmiller dustinmiller merged commit 7fd313c into master Sep 13, 2022
@dustinmiller dustinmiller deleted the rh-bond1-fixup-and-more branch September 13, 2022 18:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants