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

Bug 1912571: libvirt: support setting network dnsmasq options through the install config #4494

Merged
merged 3 commits into from
Feb 12, 2021

Conversation

Prashanth684
Copy link
Contributor

@Prashanth684 Prashanth684 commented Dec 18, 2020

Since libvirt 5.6.0, there is an option to pass in dnsmasq options through the libvirt network [1]. This addresses the following problems:

  • eliminate the need for hacking routes in the cluster (the workaround mentioned in [3]) so that libvirt's dnsmasq does not manage the domain (and so the requests from inside the cluster will go up the chain to the host itself).
  • eliminate the hacky workaround used in the multi-arch CI automation to inject *.apps entries in the libvirt network that point to a single worker node [2]. Instead of waiting for the libvirt networks to come up and update entries, we can set this before the installation itself through the install config.
  • another issue this solves - with the above mentioned workaround, having multiple worker nodes becomes problematic when running upgrade tests. Having the route to just one worker node would fail the upgrade when that worker node is down. With this change, we could now point to the .1 address and have a load balancer forward traffic to any worker node.

With this change, the option can be specified through the install config yaml in the network section as pairs of option name and values. An example:

   platform:
     libvirt:
       network:
         dnsmasqOptions:
         - name: "address"
           value: "/.apps.tt.testing/192.168.126.51"
         if: tt0

The terraform provider supports rendering these options through a datasource and injecting them into the network xml. Since this config is optional, not specifying it will continue to work as before without issues.

[1] https://libvirt.org/formatnetwork.html#elementsNamespaces
[2] https://github.com/openshift/release/blob/master/ci-operator/templates/openshift/installer/cluster-launch-installer-remote-libvirt-e2e.yaml#L532-L554
[3] #1007

@jaypoulz
Copy link
Contributor

I was looking into this yesterday when I discovered that 15 of the tests we've disabled depend on dynamic routes. :)
Glad to see you beat me to it. :)

@openshift-merge-robot
Copy link
Contributor

@Prashanth684: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
ci/prow/e2e-libvirt 6ec0031 link /test e2e-libvirt

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@Prashanth684
Copy link
Contributor Author

/test e2e-libvirt

@Prashanth684
Copy link
Contributor Author

/retest

}

// TFVarsSources contains the parameters to be converted into Terraform variables
type TFVarsSources struct {
Copy link
Member

Choose a reason for hiding this comment

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

👍 for moving that to a struct, it makes it easily readable.

@EmilienM
Copy link
Member

small nit wrt naming, otherwise lgtm.

@Prashanth684
Copy link
Contributor Author

/cc @praveenkumar @cfergeau

@Prashanth684
Copy link
Contributor Author

/retest

Copy link
Contributor

@cfergeau cfergeau 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 apart from a few minor comments. Not very familiar with the terraform plumbing, but since this code has been tested, it's fine :)

data/data/libvirt/main.tf Show resolved Hide resolved
pkg/types/libvirt/platform.go Outdated Show resolved Hide resolved
data/data/libvirt/variables-libvirt.tf Show resolved Hide resolved
…b60d7626ff8

This picks up the latest changes in the terraform provider which has support for specifying dnsmasq options
This adds the dnsmasq options block and the corresponding datasource to the libvirt terraform files
@Prashanth684 Prashanth684 changed the title libvirt: support setting network dnsmasq options through the install config Bug 1912571: libvirt: support setting network dnsmasq options through the install config Jan 4, 2021
@openshift-ci-robot
Copy link
Contributor

@Prashanth684: This pull request references Bugzilla bug 1912571, which is invalid:

  • expected the bug to target the "4.7.0" release, but it targets "---" instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

Bug 1912571: libvirt: support setting network dnsmasq options through the install config

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openshift-ci-robot openshift-ci-robot added bugzilla/severity-medium Referenced Bugzilla bug's severity is medium for the branch this PR is targeting. bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. labels Jan 4, 2021
@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 11, 2021
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

17 similar comments
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@yselkowitz
Copy link
Contributor

/bugzilla refresh

@openshift-ci-robot
Copy link
Contributor

@yselkowitz: This pull request references Bugzilla bug 1912571, which is valid.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target release (4.8.0) matches configured target release for branch (4.8.0)
  • bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

/bugzilla refresh

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

2 similar comments
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Feb 12, 2021

@Prashanth684: The following tests failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
ci/prow/e2e-crc 3c5d034 link /test e2e-crc
ci/prow/e2e-openstack a6ef6c7 link /test e2e-openstack

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit 6b89cd1 into openshift:master Feb 12, 2021
@openshift-ci-robot
Copy link
Contributor

@Prashanth684: All pull requests linked via external trackers have merged:

Bugzilla bug 1912571 has been moved to the MODIFIED state.

In response to this:

Bug 1912571: libvirt: support setting network dnsmasq options through the install config

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. bugzilla/severity-medium Referenced Bugzilla bug's severity is medium for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants