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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

馃悰 Fix IMAGE_ID issue in examples #1337

Merged
merged 2 commits into from
Nov 7, 2019

Conversation

ncdc
Copy link
Contributor

@ncdc ncdc commented Nov 7, 2019

What this PR does / why we need it:
Remove the ${IMAGE_ID} placeholder from the examples, as it's only used
for e2e conformance testing, and it's broken for example usage.

Use kustomize patches to handle the IMAGE_ID for e2e testing instead.

Cherry-pick of #1336 for master

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

@ncdc ncdc added kind/bug Categorizes issue or PR as related to a bug. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. labels Nov 7, 2019
@ncdc ncdc added this to the v0.5.0 milestone Nov 7, 2019
@ncdc ncdc requested review from dims and detiber November 7, 2019 16:05
@ncdc ncdc assigned dims and detiber Nov 7, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ncdc

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:

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

@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Nov 7, 2019
@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Nov 7, 2019
@ncdc
Copy link
Contributor Author

ncdc commented Nov 7, 2019

@dims does this look ok to you?

@ncdc
Copy link
Contributor Author

ncdc commented Nov 7, 2019

/hold

# Create control plane machine.
kubectl \
	--kubeconfig=$(kind get kubeconfig-path --name="clusterapi") \
	create -f examples/_out/controlplane.yaml
error: no objects passed to create

grrr

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 7, 2019
@vincepri
Copy link
Member

vincepri commented Nov 7, 2019

FWIW, I have seen this failure in other PRs as well

@ncdc
Copy link
Contributor Author

ncdc commented Nov 7, 2019

The error: no objects passed to create was my fault. I fixed that.

The most recent failure was the control plane never came up, but in theory the Machines and AWSMachines were all created.

@ncdc
Copy link
Contributor Author

ncdc commented Nov 7, 2019

Found some things in the ec2 instance's journal. Not sure if these are relevant or red herrings:

  • secret \"webhook-server-cert\" not found" (failed volume mount)
  • secret "cert-manager-webhook-tls" not found" (failed volume mount)

@ncdc
Copy link
Contributor Author

ncdc commented Nov 7, 2019

Hmm, seeing that in master too, in https://storage.googleapis.com/kubernetes-jenkins/logs/ci-cluster-api-provider-aws-make-conformance-master/1192488049464840192/artifacts/logs/clusterapi-control-plane/journal.log

Maybe we should merge this and work on fixing whatever is keeping the apiserver from running separately?

@vincepri
Copy link
Member

vincepri commented Nov 7, 2019

/cc @dims

Copy link
Member

@vincepri vincepri left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 7, 2019
@ncdc
Copy link
Contributor Author

ncdc commented Nov 7, 2019

AHA!

controller-runtime/controller "msg"="Reconciler error" "error"="failed to create AWSMachine instance: failed to run instance: InvalidAMIID.Malformed: Invalid id: \"${IMAGE_ID}\" (expecting \"ami-...\")\n\tstatus code: 400, request id: be6ed54d-e26a-4f56-bd18-2bb0e6d0b53f"  "controller"="awsmachine" "request"={"Namespace":"default","Name":"test1-controlplane-0"}

So maybe I need to do some more work.

/lgtm cancel

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 7, 2019
Remove the ${IMAGE_ID} placeholder from the examples, as it's only used
for e2e conformance testing, and it's broken for example usage.

Use kustomize patches to handle the IMAGE_ID for e2e testing instead.

Signed-off-by: Andy Goldstein <goldsteina@vmware.com>
(cherry picked from commit 0681bee)
Signed-off-by: Andy Goldstein <goldsteina@vmware.com>
We can't have create-cluster-management have generate-examples as a
prerequisite because our e2e conformance script calls generate-examples
standalone (with variables), and then later calls
create-cluster-management. If generate-examples is a prereq, when e2e
conformance calls create-cluster-management, it will run
generate-examples a 2nd time, but without the proper variables.

Signed-off-by: Andy Goldstein <goldsteina@vmware.com>
@ncdc
Copy link
Contributor Author

ncdc commented Nov 7, 2019

/test pull-cluster-api-provider-aws-make-conformance

@dims
Copy link
Member

dims commented Nov 7, 2019

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 7, 2019
.PHONY: create-cluster-management
create-cluster-management: $(CLUSTERCTL) generate-examples ## Create a development Kubernetes cluster on AWS in a KIND management cluster.
create-cluster-management: $(CLUSTERCTL) ## Create a development Kubernetes cluster on AWS in a KIND management cluster.
@if [[ ! -f examples/_out/cert-manager.yaml ]]; then echo "Examples are missing. Run 'make generate-examples' first."; exit 1; fi
Copy link
Member

Choose a reason for hiding this comment

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

Legit! +1 :)

@ncdc
Copy link
Contributor Author

ncdc commented Nov 7, 2019

Working!
/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 7, 2019
@k8s-ci-robot k8s-ci-robot merged commit 3ede0e1 into kubernetes-sigs:master Nov 7, 2019
@ncdc ncdc deleted the fix-examples branch November 7, 2019 21:27
@k8s-ci-robot
Copy link
Contributor

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

Test name Commit Details Rerun command
pull-cluster-api-provider-aws-make-conformance 6a0a88d link /test pull-cluster-api-provider-aws-make-conformance

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.

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. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants