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

ztp: Add ztp-deploy-wave annotation in source CR #862

Merged

Conversation

Missxiaoguo
Copy link
Contributor

@Missxiaoguo Missxiaoguo commented Dec 13, 2021

Add 'ran.openshift.io/ztp-deploy-wave' annotation in source CR
to indicate its deploy order. It will be used to set policy's
'ran.openshift.io/ztp-deploy-wave' annotation.

A check added in Makefile to make sure our provided sourceCRs have this wave annotation.

Resources have same wave means they can be applied in one policy.
Resources have different waves means they need to be applied in order and in different policies.

The default resource waves are set according to PR836 which are based on common, group-du and site. We can adjust the default waves if some resources in one group(common/group-du/site) need to be applied in order.

Wave 1(common):
Catalogsources
DisconnectedICSP
Disableoperatorhub
ReduceMonitoringFootprint

Wave 2(common):
OperatorNamespaces
OperatorGroups
OperatorSubscriptions
 
Wave 10(group-du):
ClusterLogForwarder
ClusterLogging
consoleOperatorDisable
ptpConfigSlave
ptpConfigMaster
ptpConfigSlaveCvl
ptpOperatorConfigForEvent
sriovOperatorConfig
DisableSnoNetworkDiag
StorageLV
 
 Wave 100(site specifics):
Amqinstance
performanceProfile
SriovNetwork
SriovNetworkNodePolicy
TunedPerformancePatch

Jira: https://issues.redhat.com/browse/CNF-3635
Signed-off-by: Angie Wang angwang@redhat.com
/cc @imiller0 @serngawy @browsell

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Dec 13, 2021

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 13, 2021
@openshift-ci openshift-ci bot requested review from lack and yuvalk December 13, 2021 19:45
@Missxiaoguo Missxiaoguo marked this pull request as ready for review December 13, 2021 20:05
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 13, 2021
@@ -3,6 +3,8 @@ kind: Subscription
metadata:
name: cluster-logging
namespace: openshift-logging
annotations:
ran.openshift.io/ztp-deploy-wave: "1"

Choose a reason for hiding this comment

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

Need to confirm that a subscription will eventually get reconciled if the ns/operatorgroup has not. This use to be a problem need to confirm it has been resolved

@browsell
Copy link

See the comment I made in 836. I think we want to seperate config from s/w installation, i.e. the subscription related cr's have their own policy

@@ -9,6 +9,16 @@ test-policygen:
checkExtraManifests:
$(MAKE) -C ./extra-manifests-builder check

checkSourceCRsAnnotation:
for sourcefile in ./source-crs/*.yaml;do \
if [[ "$${sourcefile}" != *"MachineConfig"* ]];then \
Copy link
Member

Choose a reason for hiding this comment

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

Why not check MachineConfig files?

We have symlinks in source-crs for backwards compatibility, so it's possible users might still include them within as policy.

What happens if there's no annotation on a CR in a PGT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Based on the discussion this morning, no need waves for MCs we want to deprecate the use of MCs after installation via policies. We need to consider how to deprecate.

To the question that "What happens if there's no annotation on a CR in a PGT"
One policy should only have sources with same waves, otherwise, it's an error. This includes a CR doesn't have wave but others have.
If policy has no wave which means its sourceCRs do not have wave, this policy will not be managed by lifecycle operator.

Add 'ran.openshift.io/ztp-deploy-wave' annotation in source CR
to indicate its deploy order. It will be used to set policy's
'ran.openshift.io/ztp-deploy-wave' annotation.

Signed-off-by: Angie Wang <angwang@redhat.com>
@Missxiaoguo
Copy link
Contributor Author

See the comment I made in 836. I think we want to seperate config from s/w installation, i.e. the subscription related cr's have their own policy

updated

Copy link
Contributor

@imiller0 imiller0 left a comment

Choose a reason for hiding this comment

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

/lgtm
On the question on subscriptions resolving when namespace and operator group not yet in place. I believe this is referencing BZ 1960455 which was fixed and verified in 4.9.

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Dec 17, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Dec 17, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: imiller0, Missxiaoguo

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 17, 2021
@openshift-merge-robot openshift-merge-robot merged commit bcad6b3 into openshift-kni:master Dec 17, 2021
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.

None yet

5 participants