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

*: Bump to install-config v0.12.0 #222

Merged
merged 4 commits into from
Feb 21, 2019

Conversation

wking
Copy link
Member

@wking wking commented Feb 14, 2019

Catching up with openshift/installer@dafc79f0 (openshift/installer#1013) and openshift/installer@3b393da8 (openshift/installer#1154).

The uint32 -> int32 cast is slightly dangerous, because it will silently wrap overflowing values (golang/go#19624, golang/go#30209). But I'll try and get the installer updated to use unsigned types as well, and then we won't have to worry about converting.

@openshift-ci-robot openshift-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Feb 14, 2019
@wking wking force-pushed the install-config-v1beta2 branch 5 times, most recently from 1a7e48d to 5207667 Compare February 14, 2019 18:47
@wking
Copy link
Member Author

wking commented Feb 14, 2019

All green except for e2e-aws (and is that actually wired up to anything in this repo?). Ping @dgoodwin

@dgoodwin
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 15, 2019
@staebler
Copy link
Contributor

staebler commented Feb 15, 2019

The uint32 -> int32 cast is slightly dangerous, because it will silently wrap overflowing values (golang/go#19624, golang/go#30209). But I'll try and get the installer updated to use unsigned types as well, and then we won't have to worry about converting.

I would prefer to see the cluster-network-operator change to int32, to fall in line with the kubernetes API conventions.

@abhinavdahiya
Copy link
Contributor

/retest

@wking
Copy link
Member Author

wking commented Feb 15, 2019

I would prefer to see the cluster-network-operator change to int32, to fall in line with the kubernetes API conventions.

Ah, good reference. Can you file an issue in their repo?

@wking
Copy link
Member Author

wking commented Feb 16, 2019

e2e-aws:

error: error executing jsonpath "{ .items[].metadata.name }": array index out of bounds: index 0, length 0
Creating ClusterDeployment Error executing template: array index out of bounds: index 0, length 0. Printing more information for debugging the template:
	template was:
		{ .items[].metadata.name }
	object given to jsonpath engine was:
		map[string]interface {}{"kind":"List", "apiVersion":"v1", "metadata":map[string]interface {}{"selfLink":"", "resourceVersion":""}, "items":[]interface {}{}}-1
Error from server (Invalid): Secret "Error executing template: array index out of bounds: index 0, length 0. Printing more information for debugging the template:\n\ttemplate was:\n\t\t{ .items[].metadata.name }\n\tobject given to jsonpath engine was:\n\t\tmap[string]interface {}{\"kind\":\"List\", \"apiVersion\":\"v1\", \"metadata\":map[string]interface {}{\"selfLink\":\"\", \"resourceVersion\":\"\"}, \"items\":[]interface {}{}}-1-aws-creds" is invalid: [metadata.name: Invalid value: "Error executing template: array index out of bounds: index 0, length 0. Printing more information for debugging the template:\n\ttemplate was:\n\t\t{ .items[].metadata.name }\n\tobject given to jsonpath engine was:\n\t\tmap[string]interface {}{\"kind\":\"List\", \"apiVersion\":\"v1\", \"metadata\":map[string]interface {}{\"selfLink\":\"\", \"resourceVersion\":\"\"}, \"items\":[]interface {}{}}-1-aws-creds": must be no more than 253 characters, metadata.name: Invalid value: "Error executing template: array index out of bounds: index 0, length 0. Printing more information for debugging the template:\n\ttemplate was:\n\t\t{ .items[].metadata.name }\n\tobject given to jsonpath engine was:\n\t\tmap[string]interface {}{\"kind\":\"List\", \"apiVersion\":\"v1\", \"metadata\":map[string]interface {}{\"selfLink\":\"\", \"resourceVersion\":\"\"}, \"items\":[]interface {}{}}-1-aws-creds": a DNS-1123 subdomain must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character (e.g. 'example.com', regex used for validation is '[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*')]

I suspect buggy cluster-name detection feeding into here (although I'm still not clear on how Hive is getting looped in to e2e-aws).

@wking
Copy link
Member Author

wking commented Feb 16, 2019

I suspect buggy cluster-name detection...

Cluster-name detection is from here. cluster.cluster.k8s.io may not be the canonical location to look that up (not sure if that was affected by the recent cluster->machine API rename or not).

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Feb 19, 2019
@wking
Copy link
Member Author

wking commented Feb 19, 2019

With openshift/installer#1169, "cluster name" is much less of a thing than it used to be (now the focus is on the full cluster domain). And I'm not really clear on the e2e test setup; which cluster was the old oc get cluster.cluster.k8s.io ... looking at, since it ran before Hive launched the test cluster? I've pushed 4556ee5 replacing the old query with a uuidgen call to get a cluster name that is decoupled from whatever cluster we used to be looking at.

@wking
Copy link
Member Author

wking commented Feb 19, 2019

e2e:

error: unable to read image registry.svc.ci.openshift.org/ci-op-pz1hbmmb/stable@sha256:826ece38c558ba3d0a745b0f465184972363a58684776a11a1c70fb681768042: received unexpected HTTP status: 504 Gateway Time-out

which is openshift/release#2905.

/retest

@dgoodwin
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 19, 2019
@dgoodwin
Copy link
Contributor

/test e2e

@dgoodwin
Copy link
Contributor

UUIDGen is making things too long:

nal: "name" cannot be longer than 32 characters: "3948e3b5-0dcc-4f7d-8390-4795aa8d99f2-ext""
level=error msg="Error: module.vpc.aws_lb.api_internal: "name" cannot be longer than 32 characters: "3948e3b5-0dcc-4f7d-8390-4795aa8d99f2-int""
level=error msg="Error: module.vpc.aws_lb_target_group.api_external: "name" cannot be longer than 32 characters"
level=error msg="Error: module.vpc.aws_lb_target_group.api_internal: "name" cannot be longer than 32 characters"
level=error msg="Error: module.vpc.aws_lb_target_group.services: "name" cannot be longer than 32 characters"

@staebler
Copy link
Contributor

UUIDGen is making things too long:

nal: "name" cannot be longer than 32 characters: "3948e3b5-0dcc-4f7d-8390-4795aa8d99f2-ext""
level=error msg="Error: module.vpc.aws_lb.api_internal: "name" cannot be longer than 32 characters: "3948e3b5-0dcc-4f7d-8390-4795aa8d99f2-int""
level=error msg="Error: module.vpc.aws_lb_target_group.api_external: "name" cannot be longer than 32 characters"
level=error msg="Error: module.vpc.aws_lb_target_group.api_internal: "name" cannot be longer than 32 characters"
level=error msg="Error: module.vpc.aws_lb_target_group.services: "name" cannot be longer than 32 characters"

This highlights a bug that needs to be addressed in the installer as well. I will file an issue there.

@csrwng csrwng mentioned this pull request Feb 19, 2019
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Feb 19, 2019
@wking
Copy link
Member Author

wking commented Feb 19, 2019

I've pushed 4556ee5 -> 3f1539d with a narrow fix for the "UUIDs are too long" issue. Although I see #223 and #226 are up with attempts to save "pull the cluster name from... wherever we're currently pulling from" ;).

@wking
Copy link
Member Author

wking commented Feb 20, 2019

e2e:

level=error msg="\t* aws_route.igw_route: Error creating route: timeout while waiting for state to become 'success' (timeout: 2m0s)"

/retest

@dgoodwin
Copy link
Contributor

Looks like several names still too long.

@csrwng
Copy link
Contributor

csrwng commented Feb 20, 2019

@wking we need a commit like 5c4f60c otherwise the uninstall pod breaks without the --clustername flag

Catching up with openshift/installer@dafc79f0 (Generate
Network.cluster config instead of NetworkConfig.networkoperator,
2019-01-15, openshift/installer#1013), openshift/installer@3b393da8
(pkg/types/aws/machinepool: Drop IAM-role overrides, 2019-01-30,
openshift/installer#1154), and openshift/installer@9ad20c35
(pkg/destroy/aws: Remove ClusterName consumer, 2019-01-31,
openshift/installer#1170).

The uint32 -> int32 cast is slightly dangerous, because it will
silently wrap overflowing values [1,2].  But I'll try and get the
installer updated to use unsigned types as well, and then we won't
have to worry about converting.

[1]: golang/go#19624
[2]: golang/go#30209
Generated by manually updating Gopkg.toml and then running:

  $ dep ensure

with:

  $ dep version
  dep:
   version     : v0.5.0-31-g73b3afe
   build date  : 2019-02-08
   git hash    : 73b3afe
   go version  : go1.10.3
   go compiler : gc
   platform    : linux/amd64
   features    : ImportDuringSolve=false
The old cluster-name extraction was leading to errors like [1]:

  error: error executing jsonpath "{ .items[].metadata.name }": array index out of bounds: index 0, length 0
  Creating ClusterDeployment Error executing template: array index out of bounds: index 0, length 0. Printing more information for debugging the template:
    template was:
      { .items[].metadata.name }
    object given to jsonpath engine was:
      map[string]interface {}{"kind":"List", "apiVersion":"v1", "metadata":map[string]interface {}{"selfLink":"", "resourceVersion":""}, "items":[]interface {}{}}-1
  Error from server (Invalid): Secret "Error executing template: array index out of bounds: index 0, length 0. Printing more information for debugging the template:\n\ttemplate was:\n\t\t{ .items[].metadata.name }\n\tobject given to jsonpath engine was:\n\t\tmap[string]interface {}{\"kind\":\"List\", \"apiVersion\":\"v1\", \"metadata\":map[string]interface {}{\"selfLink\":\"\", \"resourceVersion\":\"\"}, \"items\":[]interface {}{}}-1-aws-creds" is invalid: [metadata.name: Invalid value: "Error executing template: array index out of bounds: index 0, length 0. Printing more information for debugging the template:\n\ttemplate was:\n\t\t{ .items[].metadata.name }\n\tobject given to jsonpath engine was:\n\t\tmap[string]interface {}{\"kind\":\"List\", \"apiVersion\":\"v1\", \"metadata\":map[string]interface {}{\"selfLink\":\"\", \"resourceVersion\":\"\"}, \"items\":[]interface {}{}}-1-aws-creds": must be no more than 253 characters, metadata.name: Invalid value: "Error executing template: array index out of bounds: index 0, length 0. Printing more information for debugging the template:\n\ttemplate was:\n\t\t{ .items[].metadata.name }\n\tobject given to jsonpath engine was:\n\t\tmap[string]interface {}{\"kind\":\"List\", \"apiVersion\":\"v1\", \"metadata\":map[string]interface {}{\"selfLink\":\"\", \"resourceVersion\":\"\"}, \"items\":[]interface {}{}}-1-aws-creds": a DNS-1123 subdomain must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character (e.g. 'example.com', regex used for validation is '[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*')]

We don't actually need to match anything for our cluster names, so
just use a locally-generated UUID.

The cut avoids [2]:

  level=error msg="Error: module.vpc.aws_lb.api_internal: \"name\" cannot be longer than 32 characters: \"3948e3b5-0dcc-4f7d-8390-4795aa8d99f2-int\""

although there is installer work in progress to protect from "user
supplies long cluster name".

[1]: https://storage.googleapis.com/origin-ci-test/pr-logs/pull/openshift_hive/222/pull-ci-openshift-hive-master-e2e/80/build-log.txt
[2]: https://storage.googleapis.com/origin-ci-test/pr-logs/pull/openshift_hive/222/pull-ci-openshift-hive-master-e2e/102/build-log.txt
This script has been 100644 since it landed in 8bcee7e (Add e2e test
that installs hive and creates a cluster, 2019-01-25, openshift#191).  I'm not
sure why that wasn't a problem before, but I just saw [1]:

  hack/e2e-test.sh
  make: execvp: hack/e2e-test.sh: Permission denied
  make: *** [test-e2e] Error 127

[1]: https://storage.googleapis.com/origin-ci-test/pr-logs/pull/openshift_hive/222/pull-ci-openshift-hive-master-e2e/106/build-log.txt
@wking
Copy link
Member Author

wking commented Feb 21, 2019

... we need a commit like 5c4f60c...

Thanks. Squashed in with 742d472 -> fe8c892.

@openshift-ci-robot
Copy link

@wking: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
ci/prow/e2e fe8c892 link /test e2e

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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.

@dgoodwin
Copy link
Contributor

/override e2e
/lgtm

@openshift-ci-robot
Copy link

@dgoodwin: dgoodwin unauthorized: /override is restricted to repo administrators

In response to this:

/override e2e
/lgtm

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 the lgtm Indicates that a PR is ready to be merged. label Feb 21, 2019
@dgoodwin
Copy link
Contributor

/override e2e

1 similar comment
@dgoodwin
Copy link
Contributor

/override e2e

@dgoodwin
Copy link
Contributor

/override ci/prow/e2e

@openshift-ci-robot
Copy link

@dgoodwin: Overrode contexts on behalf of dgoodwin: ci/prow/e2e

In response to this:

/override ci/prow/e2e

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-merge-robot openshift-merge-robot merged commit a9ffb5a into openshift:master Feb 21, 2019
wking added a commit to wking/openshift-installer that referenced this pull request Feb 26, 2019
I'd kept this in 3b393da (pkg/types/aws/machinepool: Drop IAM-role
overrides, 2019-01-30, openshift#1154) to support CI.  But with
openshift/release@d31f601e (ci-operator/templates/openshift: Bump
install-config.yaml to v1beta2, 2019-02-02, openshift/release#2772)
and openshift/hive@ab7ee975 (*: Bump to install-config v0.12.0,
2019-02-14, openshift/hive#222) landed, we no longer need the
workaround.
wking added a commit to wking/hive that referenced this pull request Mar 25, 2019
The uuidgen approach is from fce3741 (hack/e2e-test: Use uuidgen to
create the cluster name, 2019-02-19, openshift#222), where we were working
around a broken oc call and the installer's poor handling of long
cluster names.  The long-name handling has since been fixed, so this
commit drops the workaround 'cut'.  It also adds a 'hive-' prefix to
make it easier to track down issues when we leak CI resources.
@wking wking deleted the install-config-v1beta2 branch September 10, 2019 14:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants