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

OCPBUGS-36553: VSphere: add vips to machinenetwork #9526

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

OCPBUGS-36553: VSphere: add vips to machinenetwork #9526

wants to merge 2 commits into from

Conversation

patrickdillon
Copy link
Contributor

Appends the ingress and api vips to the machinenetworks in the cluster infrastructure object, because the cluster network operator checks to ensure that the machinenetwork contains those vips. In some edge cases we have seen valid configurations fail when the VIPs appear to be outside the valid range.

Here are some dumps of what the infrastructure manifest looks like (these were captured during unit tests):

Single Stack

Spec:

    APIServerInternalIPs: ([]v1.IP) (len=1 cap=1) {
     (v1.IP) (len=11) "192.168.1.0"
    },
    IngressIPs: ([]v1.IP) (len=1 cap=1) {
     (v1.IP) (len=13) "192.168.222.4"
    },
    MachineNetworks: ([]v1.CIDR) (len=3 cap=4) {
     (v1.CIDR) (len=11) "10.0.0.0/16",
     (v1.CIDR) (len=14) "192.168.1.0/32",
     (v1.CIDR) (len=16) "192.168.222.4/32"
    }

Status:

    APIServerInternalIP: (string) (len=11) "192.168.1.0",
    APIServerInternalIPs: ([]string) (len=1 cap=1) {
     (string) (len=11) "192.168.1.0"
    },
    IngressIP: (string) (len=13) "192.168.222.4",
    IngressIPs: ([]string) (len=1 cap=1) {
     (string) (len=13) "192.168.222.4"
    },
    NodeDNSIP: (string) "",
    LoadBalancer: (*v1.VSpherePlatformLoadBalancer)(<nil>),
    MachineNetworks: ([]v1.CIDR) (len=3 cap=4) {
     (v1.CIDR) (len=11) "10.0.0.0/16",
     (v1.CIDR) (len=14) "192.168.1.0/32",
     (v1.CIDR) (len=16) "192.168.222.4/32"
    }

Dual Stack

Spec

      APIServerInternalIPs: ([]v1.IP) (len=2 cap=2) {
       (v1.IP) (len=11) "192.168.1.0",
       (v1.IP) (len=7) "fe80::1"
      },
      IngressIPs: ([]v1.IP) (len=2 cap=2) {
       (v1.IP) (len=13) "192.168.222.4",
       (v1.IP) (len=7) "fe80::2"
      },
      MachineNetworks: ([]v1.CIDR) (len=6 cap=8) {
       (v1.CIDR) (len=11) "10.0.0.0/16",
       (v1.CIDR) (len=9) "fe80::/10",
       (v1.CIDR) (len=14) "192.168.1.0/32",
       (v1.CIDR) (len=10) "fe80::1/64",
       (v1.CIDR) (len=16) "192.168.222.4/32",
       (v1.CIDR) (len=10) "fe80::2/64"
      }

Status

      APIServerInternalIP: (string) (len=11) "192.168.1.0",
      APIServerInternalIPs: ([]string) (len=2 cap=2) {
       (string) (len=11) "192.168.1.0",
       (string) (len=7) "fe80::1"
      },
      IngressIP: (string) (len=13) "192.168.222.4",
      IngressIPs: ([]string) (len=2 cap=2) {
       (string) (len=13) "192.168.222.4",
       (string) (len=7) "fe80::2"
      },
      NodeDNSIP: (string) "",
      LoadBalancer: (*v1.VSpherePlatformLoadBalancer)(<nil>),
      MachineNetworks: ([]v1.CIDR) (len=6 cap=8) {
       (v1.CIDR) (len=11) "10.0.0.0/16",
       (v1.CIDR) (len=9) "fe80::/10",
       (v1.CIDR) (len=14) "192.168.1.0/32",
       (v1.CIDR) (len=10) "fe80::1/64",
       (v1.CIDR) (len=16) "192.168.222.4/32",
       (v1.CIDR) (len=10) "fe80::2/64"
      }

/cc @mkowalski @jcpowermac @rvanderp3

@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Feb 27, 2025
@openshift-ci-robot openshift-ci-robot added the jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. label Feb 27, 2025
@openshift-ci-robot
Copy link
Contributor

@patrickdillon: This pull request references Jira Issue OCPBUGS-36553, which is invalid:

  • expected the bug to target either version "4.19." or "openshift-4.19.", but it targets "4.17.z" instead

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

The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

Appends the ingress and api vips to the machinenetworks in the cluster infrastructure object, because the cluster network operator checks to ensure that the machinenetwork contains those vips. In some edge cases we have seen valid configurations fail when the VIPs appear to be outside the valid range.

Here are some dumps of what the infrastructure manifest looks like (these were captured during unit tests):

Single Stack

Spec:

   APIServerInternalIPs: ([]v1.IP) (len=1 cap=1) {
    (v1.IP) (len=11) "192.168.1.0"
   },
   IngressIPs: ([]v1.IP) (len=1 cap=1) {
    (v1.IP) (len=13) "192.168.222.4"
   },
   MachineNetworks: ([]v1.CIDR) (len=3 cap=4) {
    (v1.CIDR) (len=11) "10.0.0.0/16",
    (v1.CIDR) (len=14) "192.168.1.0/32",
    (v1.CIDR) (len=16) "192.168.222.4/32"
   }

Status:

   APIServerInternalIP: (string) (len=11) "192.168.1.0",
   APIServerInternalIPs: ([]string) (len=1 cap=1) {
    (string) (len=11) "192.168.1.0"
   },
   IngressIP: (string) (len=13) "192.168.222.4",
   IngressIPs: ([]string) (len=1 cap=1) {
    (string) (len=13) "192.168.222.4"
   },
   NodeDNSIP: (string) "",
   LoadBalancer: (*v1.VSpherePlatformLoadBalancer)(<nil>),
   MachineNetworks: ([]v1.CIDR) (len=3 cap=4) {
    (v1.CIDR) (len=11) "10.0.0.0/16",
    (v1.CIDR) (len=14) "192.168.1.0/32",
    (v1.CIDR) (len=16) "192.168.222.4/32"
   }

Dual Stack

Spec

     APIServerInternalIPs: ([]v1.IP) (len=2 cap=2) {
      (v1.IP) (len=11) "192.168.1.0",
      (v1.IP) (len=7) "fe80::1"
     },
     IngressIPs: ([]v1.IP) (len=2 cap=2) {
      (v1.IP) (len=13) "192.168.222.4",
      (v1.IP) (len=7) "fe80::2"
     },
     MachineNetworks: ([]v1.CIDR) (len=6 cap=8) {
      (v1.CIDR) (len=11) "10.0.0.0/16",
      (v1.CIDR) (len=9) "fe80::/10",
      (v1.CIDR) (len=14) "192.168.1.0/32",
      (v1.CIDR) (len=10) "fe80::1/64",
      (v1.CIDR) (len=16) "192.168.222.4/32",
      (v1.CIDR) (len=10) "fe80::2/64"
     }

Status

     APIServerInternalIP: (string) (len=11) "192.168.1.0",
     APIServerInternalIPs: ([]string) (len=2 cap=2) {
      (string) (len=11) "192.168.1.0",
      (string) (len=7) "fe80::1"
     },
     IngressIP: (string) (len=13) "192.168.222.4",
     IngressIPs: ([]string) (len=2 cap=2) {
      (string) (len=13) "192.168.222.4",
      (string) (len=7) "fe80::2"
     },
     NodeDNSIP: (string) "",
     LoadBalancer: (*v1.VSpherePlatformLoadBalancer)(<nil>),
     MachineNetworks: ([]v1.CIDR) (len=6 cap=8) {
      (v1.CIDR) (len=11) "10.0.0.0/16",
      (v1.CIDR) (len=9) "fe80::/10",
      (v1.CIDR) (len=14) "192.168.1.0/32",
      (v1.CIDR) (len=10) "fe80::1/64",
      (v1.CIDR) (len=16) "192.168.222.4/32",
      (v1.CIDR) (len=10) "fe80::2/64"
     }

/cc @mkowalski @jcpowermac @rvanderp3

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 openshift-eng/jira-lifecycle-plugin repository.

patrickdillon
patrickdillon commented Feb 27, 2025
View reviewed changes
pkg/asset/manifests/vsphere/infrastructure.go Outdated
Comment on lines 106 to 116
func vipsToCIDRs(vips []string) []configv1.CIDR {
var cidrs []configv1.CIDR
for _, vip := range vips {
mask := "/32"
if strings.Contains(vip, ":") {
mask = "/64"
}
cidrs = append(cidrs, configv1.CIDR(vip+mask))
}
return cidrs
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

How bad is this? I have little idea what I'm doing, so please help!

Copy link
Contributor

Choose a reason for hiding this comment

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

For IPv6 let's use :128 if the desire is to add a single IP address

Copy link
Contributor

@mkowalski mkowalski Feb 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Also, the logic of checking if IP address is IPv6 by checking if it contains : is quite error-prone, but for what we are doing here it should not matter (i.e. nothing is going to break if you add /32 when you wanted to add /128)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! Updated.

@patrickdillon
Copy link
Contributor Author

/jira refresh

@openshift-ci-robot openshift-ci-robot added jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. and removed jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. labels Feb 27, 2025
@openshift-ci-robot
Copy link
Contributor

@patrickdillon: This pull request references Jira Issue OCPBUGS-36553, which is valid. The bug has been moved to the POST state.

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

Requesting review from QA contact:
/cc @gpei

In response to this:

/jira 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 openshift-eng/jira-lifecycle-plugin repository.

@openshift-ci openshift-ci bot requested a review from gpei February 27, 2025 21:34
@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Feb 27, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from patrickdillon. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@patrickdillon
VSphere: add vips to machinenetwork
c695a1f 
Appends the ingress and api vips to the machinenetworks in the cluster
infrastructure object, because the cluster network operator checks to
ensure that the machinenetwork contains those vips. In some edge
cases we have seen valid configurations fail when the VIPs appear
to be outside the valid range.
@patrickdillon
vSphere cluster infra object: remove deprecated VIP
42e61c6 
Removes usage of the deprecated APIVIP and IngressVIP fields.
@patrickdillon
Copy link
Contributor Author

Added a new commit to remove the deprecated APIVIP & IngressVIP fields.

@patrickdillon
Copy link
Contributor Author

/retest-required

@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Mar 6, 2025

@patrickdillon: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e-azure-ovn-resourcegroup 42e61c6 link false /test e2e-azure-ovn-resourcegroup
ci/prow/okd-scos-e2e-aws-ovn 42e61c6 link false /test okd-scos-e2e-aws-ovn
ci/prow/e2e-vsphere-ovn 42e61c6 link true /test e2e-vsphere-ovn

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-sigs/prow repository. I understand the commands that are listed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jcpowermac jcpowermac Awaiting requested review from jcpowermac

@mkowalski mkowalski Awaiting requested review from mkowalski

@rvanderp3 rvanderp3 Awaiting requested review from rvanderp3

@gpei gpei Awaiting requested review from gpei

Assignees
No one assigned
Labels
jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@patrickdillon @openshift-ci-robot @mkowalski