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

feature: add hosted mode option for join command #289

Merged
merged 1 commit into from
Feb 20, 2023
Merged

feature: add hosted mode option for join command #289

merged 1 commit into from
Feb 20, 2023

Conversation

ycyaoxdu
Copy link
Member

Signed-off-by: ycyaoxdu [email protected]

@openshift-ci openshift-ci bot requested review from itdove and qiujian16 November 21, 2022 02:29
@ycyaoxdu
Copy link
Member Author

related to #267

@ycyaoxdu
Copy link
Member Author

use --mode to set deploy mode and --external-managedcluster-kubeconfig to set external managed cluster kubeconfig.

@ycyaoxdu ycyaoxdu force-pushed the enhance-hosted-mode branch 2 times, most recently from 9a844e9 to 01f7dbc Compare November 21, 2022 02:42
@ycyaoxdu ycyaoxdu force-pushed the enhance-hosted-mode branch from 01f7dbc to 41f7951 Compare November 30, 2022 09:36
@ycyaoxdu
Copy link
Member Author

ycyaoxdu commented Nov 30, 2022 •
edited
Loading

/assign @qiujian16 @elgnay please take a look

qiujian16
qiujian16 reviewed Dec 2, 2022
View reviewed changes
pkg/cmd/join/exec.go Outdated Show resolved Hide resolved
pkg/cmd/join/exec.go Show resolved Hide resolved
pkg/cmd/join/scenario/join/default/klusterlets.cr.yaml Outdated Show resolved Hide resolved
pkg/cmd/join/scenario/join/hosted/namespace.yaml Outdated Show resolved Hide resolved
pkg/cmd/join/scenario/join/hosted/namespace.yaml Outdated Show resolved Hide resolved
@ycyaoxdu ycyaoxdu force-pushed the enhance-hosted-mode branch 2 times, most recently from fb4320f to 3e50699 Compare December 5, 2022 14:18
@ycyaoxdu ycyaoxdu requested review from qiujian16 and removed request for itdove December 9, 2022 12:19
@ycyaoxdu ycyaoxdu force-pushed the enhance-hosted-mode branch from 3e50699 to b8c1a45 Compare December 28, 2022 09:29
@ycyaoxdu ycyaoxdu changed the title feature: add hosted mode option for join command [wip]feature: add hosted mode option for join command Jan 3, 2023
@ycyaoxdu ycyaoxdu force-pushed the enhance-hosted-mode branch 3 times, most recently from 103c4f8 to 4c2c5b9 Compare January 4, 2023 06:30
@ycyaoxdu ycyaoxdu changed the title [wip]feature: add hosted mode option for join command feature: add hosted mode option for join command Jan 4, 2023
qiujian16
qiujian16 reviewed Jan 9, 2023
View reviewed changes
pkg/cmd/join/cmd.go Outdated Show resolved Hide resolved
@ycyaoxdu
Copy link
Member Author

#298

@ycyaoxdu ycyaoxdu force-pushed the enhance-hosted-mode branch from 4c2c5b9 to d43059c Compare January 10, 2023 06:46
@ycyaoxdu ycyaoxdu requested review from qiujian16 January 11, 2023 04:53
@qiujian16 qiujian16 mentioned this pull request Jan 13, 2023
Merged
@zhujian7
Copy link
Member

I think we need to take a look at the others related commands.
for example the output of the clusteradm get token contains please log on spoke and run:, should we addon more description when it is hosted mode, the join command should be run on the hosting cluster instead of the spoke cluster?

zhujian7
zhujian7 reviewed Jan 16, 2023
View reviewed changes
pkg/cmd/join/cmd.go Outdated Show resolved Hide resolved
@ycyaoxdu
Copy link
Member Author

Talked with @zhujian7 , the result is:

  1. we can not directly set the flag --klusterlet-name to <managedcluster name> . Here is a case offered by @zhujian7 : if we use clusterA as hub cluster and management cluster and set the flag --klusterlet-name to <managedcluster name>, the klusterlet name(as well as the agent namespace) is same as managedcluster name, this will leading to the two group of resources(managedcluster related and klusterlet related) stores in the same namespace, and while detaching, the namespace will be deleted, causing the managedcluster resources will be deleted unexpectedly;
  2. the flag --klusterlet-namespace can be removed, the name of this flag is misleading and we can just set the value to open-cluster-management-<klusterlet name> in our code.

@qiujian16 do you have any suggestion?

@qiujian16
Copy link
Member

qiujian16 commented Jan 19, 2023 •
edited
Loading

maybe the combination of the cluster name as a prefix and a spec hash of klusterlet if it s not set?

zhujian7
zhujian7 reviewed Jan 30, 2023
View reviewed changes
pkg/cmd/join/exec.go Outdated Show resolved Hide resolved
@ycyaoxdu ycyaoxdu changed the title feature: add hosted mode option for join command [WIP]feature: add hosted mode option for join command Jan 30, 2023
@ycyaoxdu ycyaoxdu force-pushed the enhance-hosted-mode branch from 1c76a1a to 41b7344 Compare February 9, 2023 07:08
@ycyaoxdu ycyaoxdu force-pushed the enhance-hosted-mode branch from 41b7344 to d121923 Compare February 13, 2023 13:27
@ycyaoxdu ycyaoxdu changed the title [WIP]feature: add hosted mode option for join command feature: add hosted mode option for join command Feb 13, 2023
@ycyaoxdu ycyaoxdu force-pushed the enhance-hosted-mode branch from d121923 to ef3328d Compare February 13, 2023 14:16
qiujian16
qiujian16 reviewed Feb 14, 2023
View reviewed changes
pkg/cmd/join/exec.go Outdated Show resolved Hide resolved
@ycyaoxdu
feature: add hosted mode option for join command
8a818a7 
Signed-off-by: ycyaoxdu <[email protected]>

rebase main

Signed-off-by: ycyaoxdu <[email protected]>

modify hosted mode

Signed-off-by: ycyaoxdu <[email protected]>

print preflight check results

Signed-off-by: ycyaoxdu <[email protected]>

remove klusterlet flags in hosted

Signed-off-by: ycyaoxdu <[email protected]>

refactor join command

Signed-off-by: ycyaoxdu <[email protected]>
@ycyaoxdu ycyaoxdu force-pushed the enhance-hosted-mode branch from ef3328d to 8a818a7 Compare February 17, 2023 02:35
qiujian16
qiujian16 reviewed Feb 17, 2023
View reviewed changes
Copy link
Member

@qiujian16 qiujian16 left a comment

Choose a reason for hiding this comment

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

/approve

It worth a demo on community meeting

@openshift-ci
Copy link

openshift-ci bot commented Feb 17, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: qiujian16, ycyaoxdu

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

@ycyaoxdu
Copy link
Member Author

@qiujian16 maybe after we've done more jobs in #298 ?

zhujian7
zhujian7 reviewed Feb 17, 2023
View reviewed changes
pkg/cmd/join/preflight/checks.go
}
if c.Mode == InstallModeDefault {
if c.ManagedKubeconfigFile != "" {
return nil, []error{errors.New("--managed-cluster-kubeconfig should not be set in default deploy mode")}
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 if we do need this check.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we should not set the --managed-cluster-kubeconfig with default mode. Can it be set in default mode? @zhujian7

Copy link
Member

Choose a reason for hiding this comment

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

I was thinking just ignoring it in default mode, but reporting an error also makes sense.

@zhujian7
Copy link
Member

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Feb 20, 2023
@openshift-merge-robot openshift-merge-robot merged commit accd12a into open-cluster-management-io:main Feb 20, 2023
@rolandkool rolandkool mentioned this pull request Jun 23, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@qiujian16 qiujian16 qiujian16 left review comments

@zhujian7 zhujian7 zhujian7 left review comments

Assignees

@qiujian16 qiujian16

@zhujian7 zhujian7

@elgnay elgnay

Labels
approved lgtm
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ycyaoxdu @zhujian7 @qiujian16 @openshift-merge-robot @elgnay