-
Notifications
You must be signed in to change notification settings - Fork 185
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
Add temporary support for mce installation #1474
base: master
Are you sure you want to change the base?
Conversation
Hi @mhanss. Thanks for your PR. I'm waiting for a openshift-metal3 member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
agent/04_agent_configure.sh
Outdated
@@ -461,7 +461,10 @@ EOF | |||
function oc_mirror_mce() { | |||
tmpimageset=$(mktemp --tmpdir "mceimageset--XXXXXXXXXX") | |||
_tmpfiles="$_tmpfiles $tmpimageset" | |||
|
|||
# TODO: Remove this once LSO is published to the 4.12 catalog. | |||
if [[ ${OPENSHIFT_RELEASE_STREAM} == "4.12" || ${OPENSHIFT_RELEASE_STREAM} == "4.13" ]]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to add another conditional so this is only changed when using MCE, e.g.
if [ ! -z "${AGENT_DEPLOY_MCE}" ]; then
Also it would be useful to log a warning message so its obvious that the release stream being used is not the one the user set
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Condition added!
agent/05_agent_create_cluster.sh
Outdated
function create_lso_operator_v4_11() { | ||
if [[ ${OPENSHIFT_RELEASE_STREAM} == "4.12" || ${OPENSHIFT_RELEASE_STREAM} == "4.13" ]]; then | ||
oc delete subscription local-storage-operator -n openshift-local-storage | ||
cat > "${mceManifests}/lso_operator_4_11.yaml" << EOF |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd remove the 4_11 from the name of this file so it be made more generic if you need to this same mechanism for future releases
agent/05_agent_create_cluster.sh
Outdated
source: redhat-operators-v4-11 | ||
sourceNamespace: openshift-marketplace | ||
EOF | ||
apply_manifest "$mceManifests/lso_operator_4_11.yaml" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also remove 4_11 name here
agent/05_agent_create_cluster.sh
Outdated
spec: | ||
installPlanApproval: Automatic | ||
name: local-storage-operator | ||
source: redhat-operators-v4-11 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should use a variable for "4-11" so can use for future releases
agent/05_agent_create_cluster.sh
Outdated
function mce_prepare_postinstallation_manifests() { | ||
local mceManifests=$1 | ||
|
||
# TODO: Remove this once LSO is published to the 4.12 catalog. | ||
create_lso_operator_v4_11 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we think this same issue can happen in the future, e.g. when 4.13 is being released and its operator is not released yet we should make this generic, for example, call it "create_lp_operator_latest_available' and use a variable to contain the latest - 4.11 in this case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes have been implemented!
f591d65
to
0bdf0a2
Compare
# TODO: Remove this once LSO is published to the 4.12 catalog. | ||
local version="$(openshift_version ${OCP_DIR})" | ||
if [[ ( -n "${AGENT_DEPLOY_MCE}" ) && ( ${version} == "4.12" || ${version} == "4.13" ) ]]; then | ||
local OPENSHIFT_RELEASE_STREAM="4.11" | ||
fi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This stops us from installing OCP 4.12 and OCP 4.13 doesn't it? Not sure if this is the right approach to take here.
Can't reach out to ART and DPTP to get LSO included in earlier catalogs to fix this?
Update: After talking about this with the team we will first take this approach and move this job to a weekly with a follow up approach to try to solving the underlying problem. |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lranjbar 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 |
/ok-to-test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @mhanss for your contribution. In general, I think it would be simpler to directly hard-code the "working" configuration in the existing manifests - so that we'll have the minimum impact possible on the existing code.
Also, it would be simpler in future to throw away such temporary change once the catalog will be working.
registryPoll: | ||
interval: 10m0s | ||
EOF | ||
apply_manifest "$mceManifests/catalog_source_lso_operator.yaml" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any specific reason for not having it as a manifest asset directly under /agent/asset/mce
folder? Otherwise, also for consistency with the other manifests, I'd prefer to have it there.
I guess it could also be added directly as an extra manifest.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think putting it in /agent/asset/mce makes more sense
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@andfasano @celebdor As per the current implementation, we only need to remove the code from the current file but when we will move the files to the mce
directory then we need to remove code/files from multiple places when the operator would be available to the 4.12 catalog. Please let me know your thoughts, I am okay with both approaches.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bfournie already started an activity to move the templating via Ansible, so it'd be better if we could stay on that side for the new developments (and keep away from bash). Wouldn't be sufficient just to define the source conditionally in this case? Note: any other change could be conditionally added in the same template as well, if required
spec: | ||
installPlanApproval: Automatic | ||
name: local-storage-operator | ||
source: ${catalog_source_name} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this could be added directly in the existing manifest, in this way it seems it could be possible to simplify the code
@mhanss: PR needs rebase. 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. |
@mhanss: The following test failed, say
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. |
LSO has not been published to the 4.12 redhat-operators catalog, so it cannot be installed on OpenShift 4.12. Until this is resolved, installing LSO with 4.11 catalog as redhat-operators-v4-11.