pkg/types: Add cross-platform Networking.MachineCIDR #983

wking · 2019-01-03T09:19:47Z

The concept was not platform-specific, and it's simpler to track the cross-platform value in a single, generic location.

Spun off from #792; see the thread starting here.

CC @abhinavdahiya.

wking · 2019-01-03T19:22:39Z

tf-fmt:

$ git fetch https://github.com/openshift/installer.git pull/983/head
fatal: unable to access 'https://github.com/openshift/installer.git/': Could not resolve host: github.com

/retest

pkg/types/installconfig.go

abhinavdahiya · 2019-01-03T20:21:20Z

pkg/types/installconfig.go

@@ -96,7 +96,10 @@ type Networking struct {
 	// Type is the network type to install
 	Type netopv1.NetworkType `json:"type"`

-	// ServiceCIDR is the ip block from which to assign service IPs
+	// MachineCIDR is the IP address space from which to assign machine IPs.
+	MachineCIDR ipnet.IPNet `json:"machineCIDR"`


What about MachineNetwork to match the convention that we are moving towards https://github.com/openshift/api/blob/master/config/v1/types_network.go#L26-L40

What about MachineNetwork to match the convention...

Are you ok with me breaking convention with the other properties we have here in Networking now (which use CIDR)? If we're going to make changes to the install-config, now would be the time to do that sort of thing. Personally, I think the fact that these are properties of a Networking structure is enough namespacing, so if we don't feel a need to point out CIDR-ness, I'd be fine with just:

type Networking struct { Clusters []netopv1.ClusterNetwork `json:"clusters,omitempty"` Machine ipnet.IPNet `json:"machine"` Service ipnet.IPNet `json:"service"` Type netopv1.NetworkType `json:"type"` }

The concept was not platform-specific, and it's simpler to track the cross-platform value in a single, generic location.

wking · 2019-01-04T19:42:19Z

Rebased around #982 with 7aee83e -> 31952f1.

crawford · 2019-01-04T19:52:37Z

This approach looks good to me.

/approve

wking · 2019-01-04T20:57:47Z

images:

info: Manifests will be extracted to /tmp/release-image-0.0.1-2019-01-04-195325372869831
error: unable to connect to image repository registry.svc.ci.openshift.org/ci-op-hlnzwq9h/stable@sha256:7e7e9074ae67ef2da6d80bb27880af271e0ca2043b563c9773933738a525adfd: Get https://registry.svc.ci.openshift.org/v2/: net/http: request canceled while waiting for connection (Client.Timeout exceeded while awaiting headers)
2019/01/04 19:53:49 Container release in pod release-latest failed, exit code 1, reason Error
rpc error: code = 2 desc = oci runtime error: exec failed: container_linux.go:247: starting container process caused "process_linux.go:110: decoding init error from pipe caused \"read parent: connection reset by peer\""

/retest

wking · 2019-01-04T21:26:30Z

e2e-aws:

2019/01/04 21:09:09 Running pod e2e-aws
level=fatal msg="failed to fetch Terraform Variables: failed to load asset \"Install Config\": invalid \"install-config.yaml\" file: networking.machineCIDR: Invalid value: ipnet.IPNet{IPNet:net.IPNet{IP:net.IP(nil), Mask:net.IPMask(nil)}}: must use IPv4"

Looks like I need to wait for #902 or cludge something in...

Preparing for openshift/installer@31952f15 (pkg/types: Add cross-platform Networking.MachineCIDR, 2019-01-03, openshift/installer#983). Once that lands we can drop the vpcCIDRBlock and NetworkCIDRBlock properties.

sdodson · 2019-01-04T22:16:48Z

Does this filter down to anything that would need to support multiple CIDR blocks?

wking · 2019-01-04T22:29:04Z

Does this filter down to anything that would need to support multiple CIDR blocks?

The closest would probably using this for the libvirt domain's network here, and choose IPs from it here. But on all of our platforms, there's one network (e.g. the AWS VPC) that holds all the machines, so I expect we'll be fine with this until we get to stretch clusters or federation.

wking · 2019-01-04T22:29:54Z

openshift/release#2506 landed.

/retest

wking · 2019-01-04T23:31:09Z

Checking via https://api.ci.openshift.org/console/project/ci-op-bpcyl265/browse/pods/e2e-aws?tab=logs (because I'm the PR author), this is going to fail with at least:

fail [github.com/openshift/origin/test/extended/deployments/deployments.go:612]: Expected
    <string>: --> pre: Running hook pod ...
    test pre hook executed
    --> pre: Success
    --> Scaling custom-deployment-1 to 2
    --> Reached 50%
    Halfway
    --> pre: Hook pod already succeeded
to contain substring
  <string>: Finished
...

failed: (1m46s) 2019-01-04T23:24:47 "[Feature:DeploymentConfig] deploymentconfigs with custom deployments [Conformance] should run the custom deployment steps [Suite:openshift/conformance/parallel/minimal]"

From here, that test has been flaky for update payload promotion as well. I'll kick it again once it returns.

crawford · 2019-01-07T19:00:01Z

/lgtm

openshift-ci-robot · 2019-01-07T19:00:31Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: crawford, wking

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [crawford,wking]

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

wking · 2019-01-07T20:39:44Z

e2e-aws:

Flaky tests:

[sig-storage] Dynamic Provisioning DynamicProvisioner should test that deleting a claim before the volume is provisioned deletes the volume. [Suite:openshift/conformance/parallel] [Suite:k8s] [Suite:openshift/smoke-4]
[sig-storage] Dynamic Provisioning Invalid AWS KMS key should report an error and create no PV [Suite:openshift/conformance/parallel] [Suite:k8s]

Writing JUnit report to /tmp/artifacts/junit/junit_e2e_20190107-200258.xml

Error: 2 fail, 522 pass, 97 skip (27m37s)

/retest

wking · 2019-01-07T22:29:26Z

e2e-aws:

Flaky tests:

[Feature:DeploymentConfig] deploymentconfigs with test deployments [Conformance] should run a deployment to completion and then scale to zero [Suite:openshift/conformance/parallel/minimal]
[sig-auth] ServiceAccounts should allow opting out of API token automount  [Conformance] [Suite:openshift/conformance/parallel/minimal] [Suite:k8s]

Writing JUnit report to /tmp/artifacts/junit/junit_e2e_20190107-212827.xml

Error: 2 fail, 522 pass, 97 skip (24m39s)

/retest

wking · 2019-01-08T00:21:08Z

e2e-aws:

Flaky tests:

[Feature:DeploymentConfig] deploymentconfigs with test deployments [Conformance] should run a deployment to completion and then scale to zero [Suite:openshift/conformance/parallel/minimal]
[sig-auth] ServiceAccounts should allow opting out of API token automount  [Conformance] [Suite:openshift/conformance/parallel/minimal] [Suite:k8s]

Writing JUnit report to /tmp/artifacts/junit/junit_e2e_20190107-231944.xml

Error: 2 fail, 522 pass, 97 skip (25m39s)

Same failures as last time? Hmm.

/retest

wking · 2019-01-08T02:00:01Z

Hooray, finally got through CI 🎉

dgoodwin · 2019-01-08T12:41:18Z

This change broke Hive, whenever possible please let us know if a non-backward compat change is made to install config. Will update to fix.

wking · 2019-01-08T14:00:07Z

This change broke Hive, whenever possible please let us know if a non-backward compat change is made to install config.

I pinged @vrutkovs about the change here (although I could have done a better job emphasizing breaking-ness). Which other users/teams want to be CCed for future breaks? (although we're almost through the time when we're allowed to break this)

dgoodwin · 2019-01-08T14:03:06Z

Things are crazy and stuff gets missed, I can understand, but yeah hopefully all of this can stabilize soon.

This pull request removed the platform-specific `openstack.NetworkCIDRBlock` and friends in favour of a cross-platform `networking.machineCIDR`: openshift/installer#983 Deployments using the old NetworkCIDRBlock are failing now.

Now that openshift/installer@31952f15 (pkg/types: Add cross-platform Networking.MachineCIDR, 2019-01-03, openshift/installer#983) has landed, we can drop the vpcCIDRBlock and NetworkCIDRBlock properties. This completes the transition started by 2a9a6a3 (ci-operator/templates/openshift: Set machineCIDR, 2019-01-04, openshift#2506).

openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jan 3, 2019

openshift-ci-robot requested review from flaper87 and hardys January 3, 2019 09:20

wking force-pushed the platform-agnostic-machine-cidr branch from 544cf18 to 7156171 Compare January 3, 2019 09:27

wking mentioned this pull request Jan 3, 2019

pkg/tfvars: Pull from []Machine instead of InstallConfig #792

Merged

wking force-pushed the platform-agnostic-machine-cidr branch from 7156171 to f27c67b Compare January 3, 2019 20:03

abhinavdahiya reviewed Jan 3, 2019

View reviewed changes

wking force-pushed the platform-agnostic-machine-cidr branch from f27c67b to 7aee83e Compare January 3, 2019 23:40

pkg/types: Add cross-platform Networking.MachineCIDR

31952f1

The concept was not platform-specific, and it's simpler to track the cross-platform value in a single, generic location.

wking force-pushed the platform-agnostic-machine-cidr branch from 7aee83e to 31952f1 Compare January 4, 2019 19:41

wking mentioned this pull request Jan 4, 2019

ci-operator/templates/openshift: Set machineCIDR openshift/release#2506

Merged

openshift-ci-robot assigned crawford Jan 7, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 7, 2019

openshift-merge-robot merged commit 87a44e7 into openshift:master Jan 8, 2019

wking deleted the platform-agnostic-machine-cidr branch January 8, 2019 02:02

sallyom mentioned this pull request Jan 8, 2019

add machineCIDR to install-config ironcladlou/openshift4-libvirt-gcp#9

Merged

vrutkovs mentioned this pull request Jan 8, 2019

BYOR 4.0: specify machineCIDR in openshift-ansible install config openshift/release#2523

Merged

tomassedovic mentioned this pull request Jan 8, 2019

Use networking.machineCIDR imain/ocp-doit#36

Merged

wking mentioned this pull request Jan 10, 2019

ci-operator/templates/openshift: Finish machineCIDR transition openshift/release#2549

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pkg/types: Add cross-platform Networking.MachineCIDR #983

pkg/types: Add cross-platform Networking.MachineCIDR #983

wking commented Jan 3, 2019

wking commented Jan 3, 2019

abhinavdahiya Jan 3, 2019

wking Jan 3, 2019

wking commented Jan 4, 2019

crawford commented Jan 4, 2019

wking commented Jan 4, 2019

wking commented Jan 4, 2019

sdodson commented Jan 4, 2019

wking commented Jan 4, 2019

wking commented Jan 4, 2019

wking commented Jan 4, 2019

crawford commented Jan 7, 2019

openshift-ci-robot commented Jan 7, 2019

wking commented Jan 7, 2019

wking commented Jan 7, 2019

wking commented Jan 8, 2019

wking commented Jan 8, 2019 •

edited

Loading

dgoodwin commented Jan 8, 2019

wking commented Jan 8, 2019

dgoodwin commented Jan 8, 2019

pkg/types: Add cross-platform Networking.MachineCIDR #983

pkg/types: Add cross-platform Networking.MachineCIDR #983

Conversation

wking commented Jan 3, 2019

wking commented Jan 3, 2019

abhinavdahiya Jan 3, 2019

Choose a reason for hiding this comment

wking Jan 3, 2019

Choose a reason for hiding this comment

wking commented Jan 4, 2019

crawford commented Jan 4, 2019

wking commented Jan 4, 2019

wking commented Jan 4, 2019

sdodson commented Jan 4, 2019

wking commented Jan 4, 2019

wking commented Jan 4, 2019

wking commented Jan 4, 2019

crawford commented Jan 7, 2019

openshift-ci-robot commented Jan 7, 2019

wking commented Jan 7, 2019

wking commented Jan 7, 2019

wking commented Jan 8, 2019

wking commented Jan 8, 2019 • edited Loading

dgoodwin commented Jan 8, 2019

wking commented Jan 8, 2019

dgoodwin commented Jan 8, 2019

wking commented Jan 8, 2019 •

edited

Loading