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

MGMT-3370: Allow running in kube api mode #505

Closed
wants to merge 1 commit into from

Conversation

RazRegev
Copy link
Contributor

@RazRegev RazRegev commented Feb 8, 2021

By passing ENABLE_KUBE_API env.
Use the skipper image of assisted-service which has all
the relevant dependencies required to generate and deploy
relevant definitions and deploy the service with the kube
controller enabled.

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 8, 2021
@RazRegev RazRegev force-pushed the MGMT-3370-add-kubeapi branch from 1f1a6f6 to 5075650 Compare February 8, 2021 12:17
@tsorya
Copy link
Contributor

tsorya commented Feb 8, 2021

/approve
/lgtm

@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: RazRegev, tsorya

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-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 8, 2021
@@ -50,7 +51,7 @@ elif [ "${DEPLOY_TARGET}" == "ocp" ]; then
else
print_log "Updating assisted_service params"
skipper run discovery-infra/update_assisted_service_cm.py ENABLE_AUTH=${ENABLE_AUTH}
skipper run "make -C assisted-service/ deploy-all" ${SKIPPER_PARAMS} DEPLOY_TAG=${DEPLOY_TAG} DEPLOY_MANIFEST_PATH=${DEPLOY_MANIFEST_PATH} DEPLOY_MANIFEST_TAG=${DEPLOY_MANIFEST_TAG} NAMESPACE=${NAMESPACE} ENABLE_AUTH=${ENABLE_AUTH} PROFILE=${PROFILE}
cd assisted-service/ && skipper run "make deploy-all" ${SKIPPER_PARAMS} ENABLE_KUBE_API=${ENABLE_KUBE_API} DEPLOY_TAG=${DEPLOY_TAG} DEPLOY_MANIFEST_PATH=${DEPLOY_MANIFEST_PATH} DEPLOY_MANIFEST_TAG=${DEPLOY_MANIFEST_TAG} NAMESPACE=${NAMESPACE} ENABLE_AUTH=${ENABLE_AUTH} PROFILE=${PROFILE}
Copy link
Contributor

Choose a reason for hiding this comment

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

why do you cd to the directory and not running skipper run "make -C assisted-service/ deploy-all" ?

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 reason I did it because it currently runs with a test-infra skipper image.
If we want to have the dependencies to generate manifests we need to use an assisted-service build image.
I think regardless of this specific need that this would be more appropriate to use an assisted-service build image to deploy assisted-service.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is it because of the controller-gen?
@tsorya is there a specific reason why assisted build image wasn't used to begin with?

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, the controller gen is the main reason

Copy link
Contributor

Choose a reason for hiding this comment

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

sounds logical i must say but it will require to create 2 build images in every deployment and will take very long time

Copy link
Contributor

Choose a reason for hiding this comment

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

Then to make it work with single build container we need to install some things from this commit openshift/assisted-service@697a281
Do you think it will better?

scripts/deploy_assisted_service.sh Outdated Show resolved Hide resolved
@tsorya
Copy link
Contributor

tsorya commented Feb 8, 2021

/hold

@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 Feb 8, 2021
By passing ENABLE_KUBE_API env.
Use the skipper image of assisted-service which has all
the relevant dependencies required to generate and deploy
relevant definitions and deploy the service with the kube
controller enabled.
@RazRegev RazRegev force-pushed the MGMT-3370-add-kubeapi branch from 5075650 to d52ccf0 Compare February 8, 2021 13:30
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Feb 8, 2021
@openshift-ci-robot
Copy link

New changes are detected. LGTM label has been removed.

@openshift-ci
Copy link

openshift-ci bot commented Feb 8, 2021

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

Test name Commit Details Rerun command
ci/prow/unit d52ccf0 link /test unit
ci/prow/e2e-metal-single-node-live-iso d52ccf0 link /test e2e-metal-single-node-live-iso

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.

scripts/deploy_assisted_service.sh Show resolved Hide resolved
@@ -50,7 +53,7 @@ elif [ "${DEPLOY_TARGET}" == "ocp" ]; then
else
print_log "Updating assisted_service params"
skipper run discovery-infra/update_assisted_service_cm.py ENABLE_AUTH=${ENABLE_AUTH}
skipper run "make -C assisted-service/ deploy-all" ${SKIPPER_PARAMS} DEPLOY_TAG=${DEPLOY_TAG} DEPLOY_MANIFEST_PATH=${DEPLOY_MANIFEST_PATH} DEPLOY_MANIFEST_TAG=${DEPLOY_MANIFEST_TAG} NAMESPACE=${NAMESPACE} ENABLE_AUTH=${ENABLE_AUTH} PROFILE=${PROFILE}
cd assisted-service/ && skipper run "make deploy-all" ${SKIPPER_PARAMS} $ENABLE_KUBE_API_CMD DEPLOY_TAG=${DEPLOY_TAG} DEPLOY_MANIFEST_PATH=${DEPLOY_MANIFEST_PATH} DEPLOY_MANIFEST_TAG=${DEPLOY_MANIFEST_TAG} NAMESPACE=${NAMESPACE} ENABLE_AUTH=${ENABLE_AUTH} PROFILE=${PROFILE}
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe wrap it all with parenthesis so that the cd is relevant to this line alone

Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of doing this, maybe lets install the dependencies https://github.com/openshift/assisted-service/pull/829/files and install them in test-infra.
Then we will not need two build containers.

@RazRegev
Copy link
Contributor Author

closing, I added the dependencies to the test-infra skipper after discussing with @filanov and understood these changes aren't heavy as we thought. @tsorya
#521

@RazRegev RazRegev closed this Feb 15, 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. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants