Skip to content

Conversation

@cybertron
Copy link
Contributor

This provides a way to specify a custom networkConfig for each node
in a deployment. The user sets NETWORK_CONFIG_FOLDER to a path that
contains YAML files named for each node. The contents of these files
are included in the install-config definition for the appropriate
node.

@openshift-ci openshift-ci bot requested review from flaper87 and markmc June 24, 2022 19:06
@cybertron
Copy link
Contributor Author

/hold

This is only a partial implementation (for static IPs we need a way to add DNS entries too), but I wanted to get it pushed as an example of what I was thinking for a generic networkConfig interface.

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 24, 2022
@cybertron cybertron mentioned this pull request Jun 24, 2022
@cybertron
Copy link
Contributor Author

/cc @creydr
/hold cancel

This is working for me locally. We do need to sync with @derekhiggins to see if we can implement his static ip patch using this mechanism.

I tested with this network config directory:

==> network-config-static/hosts.yaml <==
- ip: 192.168.111.110
  hostnames:
    - "master-0"
- ip: 192.168.111.111
  hostnames:
    - "master-1"
- ip: 192.168.111.112
  hostnames:
    - "master-2"
- ip: 192.168.111.113
  hostnames:
    - "worker-0"
- ip: 192.168.111.114
  hostnames:
    - "worker-1"

==> network-config-static/ostest-master-0.yaml <==
networkConfig:
  interfaces:
  - name: enp2s0
    type: ethernet
    state: up
    ipv4:
      address:
      - ip: "192.168.111.110"
        prefix-length: 24
      enabled: true
  dns-resolver:
    config:
      server:
      - 192.168.111.1
  routes:
    config:
    - destination: 0.0.0.0/0
      next-hop-address: 192.168.111.1
      next-hop-interface: enp2s0

==> network-config-static/ostest-master-1.yaml <==
networkConfig:
  interfaces:
  - name: enp2s0
    type: ethernet
    state: up
    ipv4:
      address:
      - ip: "192.168.111.111"
        prefix-length: 24
      enabled: true
  dns-resolver:
    config:
      server:
      - 192.168.111.1
  routes:
    config:
    - destination: 0.0.0.0/0
      next-hop-address: 192.168.111.1
      next-hop-interface: enp2s0

==> network-config-static/ostest-master-2.yaml <==
networkConfig:
  interfaces:
  - name: enp2s0
    type: ethernet
    state: up
    ipv4:
      address:
      - ip: "192.168.111.112"
        prefix-length: 24
      enabled: true
  dns-resolver:
    config:
      server:
      - 192.168.111.1
  routes:
    config:
    - destination: 0.0.0.0/0
      next-hop-address: 192.168.111.1
      next-hop-interface: enp2s0

==> network-config-static/ostest-worker-0.yaml <==
networkConfig:
  interfaces:
  - name: enp2s0
    type: ethernet
    state: up
    ipv4:
      address:
      - ip: "192.168.111.113"
        prefix-length: 24
      enabled: true
  dns-resolver:
    config:
      server:
      - 192.168.111.1
  routes:
    config:
    - destination: 0.0.0.0/0
      next-hop-address: 192.168.111.1
      next-hop-interface: enp2s0

==> network-config-static/ostest-worker-1.yaml <==
networkConfig:
  interfaces:
  - name: enp2s0
    type: ethernet
    state: up
    ipv4:
      address:
      - ip: "192.168.111.114"
        prefix-length: 24
      enabled: true
  dns-resolver:
    config:
      server:
      - 192.168.111.1
  routes:
    config:
    - destination: 0.0.0.0/0
      next-hop-address: 192.168.111.1
      next-hop-interface: enp2s0```

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 6, 2022
@openshift-ci
Copy link

openshift-ci bot commented Jul 6, 2022

@cybertron: GitHub didn't allow me to request PR reviews from the following users: creydr.

Note that only openshift-metal3 members and repo collaborators can review this PR, and authors cannot review their own PRs.

Details

In response to this:

/cc @creydr
/hold cancel

This is working for me locally. We do need to sync with @derekhiggins to see if we can implement his static ip patch using this mechanism.

I tested with this network config directory:

==> network-config-static/hosts.yaml <==
- ip: 192.168.111.110
 hostnames:
   - "master-0"
- ip: 192.168.111.111
 hostnames:
   - "master-1"
- ip: 192.168.111.112
 hostnames:
   - "master-2"
- ip: 192.168.111.113
 hostnames:
   - "worker-0"
- ip: 192.168.111.114
 hostnames:
   - "worker-1"

==> network-config-static/ostest-master-0.yaml <==
networkConfig:
 interfaces:
 - name: enp2s0
   type: ethernet
   state: up
   ipv4:
     address:
     - ip: "192.168.111.110"
       prefix-length: 24
     enabled: true
 dns-resolver:
   config:
     server:
     - 192.168.111.1
 routes:
   config:
   - destination: 0.0.0.0/0
     next-hop-address: 192.168.111.1
     next-hop-interface: enp2s0

==> network-config-static/ostest-master-1.yaml <==
networkConfig:
 interfaces:
 - name: enp2s0
   type: ethernet
   state: up
   ipv4:
     address:
     - ip: "192.168.111.111"
       prefix-length: 24
     enabled: true
 dns-resolver:
   config:
     server:
     - 192.168.111.1
 routes:
   config:
   - destination: 0.0.0.0/0
     next-hop-address: 192.168.111.1
     next-hop-interface: enp2s0

==> network-config-static/ostest-master-2.yaml <==
networkConfig:
 interfaces:
 - name: enp2s0
   type: ethernet
   state: up
   ipv4:
     address:
     - ip: "192.168.111.112"
       prefix-length: 24
     enabled: true
 dns-resolver:
   config:
     server:
     - 192.168.111.1
 routes:
   config:
   - destination: 0.0.0.0/0
     next-hop-address: 192.168.111.1
     next-hop-interface: enp2s0

==> network-config-static/ostest-worker-0.yaml <==
networkConfig:
 interfaces:
 - name: enp2s0
   type: ethernet
   state: up
   ipv4:
     address:
     - ip: "192.168.111.113"
       prefix-length: 24
     enabled: true
 dns-resolver:
   config:
     server:
     - 192.168.111.1
 routes:
   config:
   - destination: 0.0.0.0/0
     next-hop-address: 192.168.111.1
     next-hop-interface: enp2s0

==> network-config-static/ostest-worker-1.yaml <==
networkConfig:
 interfaces:
 - name: enp2s0
   type: ethernet
   state: up
   ipv4:
     address:
     - ip: "192.168.111.114"
       prefix-length: 24
     enabled: true
 dns-resolver:
   config:
     server:
     - 192.168.111.1
 routes:
   config:
   - destination: 0.0.0.0/0
     next-hop-address: 192.168.111.1
     next-hop-interface: enp2s0```

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.

@cybertron
Copy link
Contributor Author

This now includes all of the changes needed to deploy with bonds. It still needs documentation, but I wanted to get feedback on whether the approach is okay before I do that work.

@cybertron
Copy link
Contributor Author

/hold

I randomly discovered that this breaks the additional networks functionality needed for kubernetes-nmstate testing. I need to figure out how to make those play nicely before this goes in.

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 11, 2022
@cybertron
Copy link
Contributor Author

/hold cancel

This version plays nice with extra networks too.

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 12, 2022
@tdomnesc
Copy link
Contributor

I tried this on my dev server with: static IPs , static IPs with vlan, and also interface.ipv4.dhcp: true without a hosts.yaml file. All worked.

@elfosardo
Copy link
Member

/retest

@derekhiggins
Copy link
Collaborator

/approved

This provides a way to specify a custom networkConfig for each node
in a deployment. The user sets NETWORK_CONFIG_FOLDER to a path that
contains YAML files named for each node. The contents of these files
are included in the install-config definition for the appropriate
node.
To support custom networkConfigs that include static IPs (which
require reverse DNS records for each node so a hostname can be
assigned), this adds the ability to specify additional DNS hosts
that will be added to the regular list of hosts that dev-scripts
creates in DNS.

This config is only added if NETWORK_CONFIG_FOLDER is set and the
file 'hosts.yaml' exists in that location. The file contents should
be a list of IPs and hostnames, such as:

- ip: 192.168.111.110
  hostnames:
    - "master-0"
- ip: 192.168.111.111
  hostnames:
    - "master-1"
etc.
In order to test deployment on bonds we need a way to have two
interfaces on the baremetal network. This adds a new environment
variable, BOND_PRIMARY_INTERFACE, that adds two NICs on the baremetal
network to each node.
@cybertron
Copy link
Contributor Author

Since it seems like people are happy with this I added documentation and made the folder path a little more user-friendly. I think this should be good to go.

Also makes NETWORK_CONFIG_FOLDER able to handle relative paths.
@cybertron
Copy link
Contributor Author

/retest

1 similar comment
@cybertron
Copy link
Contributor Author

/retest

@elfosardo
Copy link
Member

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 24, 2022
@derekhiggins
Copy link
Collaborator

/approve

@openshift-ci
Copy link

openshift-ci bot commented Aug 24, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: derekhiggins

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

The pull request process is described here

Details 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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 24, 2022
@openshift-ci
Copy link

openshift-ci bot commented Aug 24, 2022

@cybertron: 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-metal-ipi-proxy-ipv4 67dfe48 link false /test e2e-metal-ipi-proxy-ipv4
ci/prow/e2e-metal-ipi-proxy-ipv6 67dfe48 link false /test e2e-metal-ipi-proxy-ipv6

Full PR test history. Your PR dashboard.

Details

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.

@cybertron
Copy link
Contributor Author

/retest

@openshift-merge-robot openshift-merge-robot merged commit f100709 into openshift-metal3:master Aug 24, 2022
cybertron added a commit to cybertron/dev-scripts that referenced this pull request Sep 21, 2022
These are some example configs that can be used with the
NETWORK_CONFIG_FOLDER option added in openshift-metal3#1418. In addition to making
it easy for new users to test the networkConfig feature, this will
also make it easier to run such configurations in CI.

To use, just set the variable to the appropriate path. For example:

export NETWORK_CONFIG_FOLDER=network-configs/static
openshift-merge-robot pushed a commit that referenced this pull request Sep 22, 2022
These are some example configs that can be used with the
NETWORK_CONFIG_FOLDER option added in #1418. In addition to making
it easy for new users to test the networkConfig feature, this will
also make it easier to run such configurations in CI.

To use, just set the variable to the appropriate path. For example:

export NETWORK_CONFIG_FOLDER=network-configs/static
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. lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants