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

Add ClusterProfile template variable #483

Merged
merged 1 commit into from Dec 10, 2020
Merged

Add ClusterProfile template variable #483

merged 1 commit into from Dec 10, 2020

Conversation

guillaumerose
Copy link
Contributor

This allows future CVO manifests to use {{ .ClusterProfile }}.

Example, futur CVO might look like:

apiVersion: v1
kind: Pod
metadata:
  name: bootstrap-cluster-version-operator
  namespace: openshift-cluster-version
...
    env:
      - name: CLUSTER_PROFILE
        value: "{{.ClusterProfile}}"

This implements phase 1 of openshift/enhancements#543.

Default value is the one decided in openshift/enhancements#510

@guillaumerose
Add ClusterProfile template variable
1816230 
This allows future CVO manifests to use {{ .ClusterProfile }}.
@eranco74 eranco74 mentioned this pull request Dec 2, 2020
Closed
@sdodson
Copy link
Member

sdodson commented Dec 3, 2020

/approve
@jottofar @wking can you PTAL today now that the enhancement has been accepted.

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 3, 2020
@jottofar
Copy link
Contributor

jottofar commented Dec 3, 2020

I don't see anything wrong with the changes.

@LalatenduMohanty
Copy link
Member

/assign @LalatenduMohanty

LalatenduMohanty
LalatenduMohanty reviewed Dec 4, 2020
View reviewed changes
pkg/payload/render.go
renderConfig = manifestRenderConfig{
ReleaseImage: releaseImage,
ClusterProfile: defaultClusterProfile,
}
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure who calls this function? Is it installer during bootstrap?

Copy link
Member

Choose a reason for hiding this comment

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

I mean Render(outputDir, releaseImage string) error

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this code https://github.com/openshift/installer/blob/master/data/data/bootstrap/files/usr/local/bin/bootkube.sh.template#L62

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The env. variable is not plugged yet. This PR just teaches enough to the CVO for updating to a CVO that uses ClusterProfile in his manifests.

Copy link
Member

Choose a reason for hiding this comment

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

Looks like the the PR for env variable already merged for installer.

@LalatenduMohanty
Copy link
Member

/retest

@LalatenduMohanty
Copy link
Member

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 4, 2020
@romfreiman
Copy link

/retest

1 similar comment
@romfreiman
Copy link

/retest

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

13 similar comments
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@romfreiman
Copy link

/retest

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@romfreiman
Copy link

/retest

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

1 similar comment
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@sdodson
Copy link
Member

sdodson commented Dec 5, 2020

/hold
Persistent failure running oc with deprecated --config flag, unless someone is fixing that retesting wont help. I'm not sure where that's being called though. @LalatenduMohanty ptal next week

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 5, 2020
@guillaumerose
Copy link
Contributor Author

I am wondering if it's due to to the usage of 4.6 sidecar in a master build:

ci-operator/jobs/openshift/cluster-version-operator/openshift-cluster-version-operator-master-presubmits.yaml
332:          subPath: master-sidecar-4.6.yaml
345:          name: prow-job-master-sidecar-4.6

@sdodson
Copy link
Member

sdodson commented Dec 5, 2020 via email

@eparis
Copy link
Member

eparis commented Dec 5, 2020

I this valuable without operator-framework/api#73 or #404 ? I was under the impression that both were pre-reqs.

@guillaumerose
Copy link
Contributor Author

@eparis manifests need to be all done for cvo#404, not this one. This one is just about fixing the next upgrade.

@guillaumerose
Copy link
Contributor Author

@sdodson I opened openshift/release#14121

ci-operator/jobs/openshift/cluster-version-operator/openshift-cluster-version-operator-master-presubmits.yaml uses master-sidecar-4.6.yaml in /test integration. I hope its the right place.

@guillaumerose
Copy link
Contributor Author

/retest

@guillaumerose
Copy link
Contributor Author

kube apiserver log: error creating self-signed certificates: mkdir /var/run/kubernetes: permission denied

Let's try again.
/retest

@guillaumerose guillaumerose mentioned this pull request Dec 8, 2020
Closed
@LalatenduMohanty
Copy link
Member

LalatenduMohanty commented Dec 8, 2020 •
edited

Integration tests failed because of error: some steps failed: could not run steps: step integration failed: template pod "integration" failed: the pod ci-op-qrj995d0/integration failed after 1m16s (failed containers: setup): ContainerFailed one or more containers exited Container setup exited with code 1, reason Error . It does not look like an issue with this PR, hence retesting.

@LalatenduMohanty
Copy link
Member

/test integration

@guillaumerose
Copy link
Contributor Author

@LalatenduMohanty Integration tests need to be changed deeper. It will not pass until. See openshift/release#13745

@LalatenduMohanty
Copy link
Member

@LalatenduMohanty Integration tests need to be changed deeper. It will not pass until. See openshift/release#13745

Looks like it is ready. We should we able to merge it soon

@wking
Copy link
Member

wking commented Dec 9, 2020

As of the current 1816230 tip, the changes in this PR are wired up to neither the CLUSTER_PROFILE environment variable that the installer recently learned how to set (openshift/installer#4444) nor the manifest loading where we'd like to perform the filtering (around here). Both of those look like they were intended to be part of phase 1. Looks like these comments touched on this issue, but I'm still not clear. Can someone explain why 1816230 alone is useful in more detail? And, if it needs to be backported to 4.6, probably put that description in the bug you file to motivate the backport.

@guillaumerose
Copy link
Contributor Author

Can someone explain why 1816230 alone is useful in more detail?

Alone, it unlocks the e2e test that checks the upgrade from an old CVO to a new CVO that uses .ClusterProfile in his manifests.
See #404 (comment)

Outgoing CVO will render the incoming CVO and launch it. The new CVO (hopefully with cvo#404) will pick the right manifests.

motivate the backport

Let's say 4.8 uses .ClusterProfile in CVO manifests. Only CVO 4.7 will be able to upgrade to 4.8. Is it a problem?

@guillaumerose
Copy link
Contributor Author

/retest

@openshift-merge-robot
Copy link
Contributor

openshift-merge-robot commented Dec 9, 2020 •
edited

@guillaumerose: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
ci/prow/integration 1816230 link /test integration

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

@wking
Copy link
Member

wking commented Dec 9, 2020

/retest

@wking
Copy link
Member

wking commented Dec 10, 2020

/lgtm

@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gbraad, guillaumerose, LalatenduMohanty, sdodson, 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 [LalatenduMohanty,sdodson,wking]

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

@LalatenduMohanty
Copy link
Member

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 10, 2020
@openshift-merge-robot openshift-merge-robot merged commit b6cfb17 into openshift:master Dec 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LalatenduMohanty LalatenduMohanty LalatenduMohanty left review comments

@gbraad gbraad gbraad approved these changes

@smarterclayton smarterclayton Awaiting requested review from smarterclayton

@wking wking Awaiting requested review from wking

Assignees

@wking wking

@LalatenduMohanty LalatenduMohanty

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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet