`karmadactl`: Fixed `--namespace` flag of `init` command not work issue. #1416

sayaoailun · 2022-03-01T03:14:19Z

Signed-off-by: sayaoailun guojianwei007@126.com

What type of PR is this?

/kind bug

What this PR does / why we need it:

The flag -n of karmadactl doesn't work as expected

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

`karmadactl`: Fixed `--namespace` flag of `init` command not work issue.

karmada-bot · 2022-03-01T03:14:29Z

Welcome @sayaoailun! It looks like this is your first PR to karmada-io/karmada 🎉

RainbowMango · 2022-03-01T03:20:28Z

The flag -n of karmadactl doesn't work as expected

Thanks, @sayaoailun for the feedback on this. How to reproduce it? Could you please give more details?

sayaoailun · 2022-03-01T05:46:03Z

The flag -n of karmadactl doesn't work as expected

Thanks, @sayaoailun for the feedback on this. How to reproduce it? Could you please give more details?

@RainbowMango

When init Karmada, set -n to any namespace except karmada-system, e.g. test-karmada.

The APIService in Karmada will be set to karmada-aggregated-apiserver.karmada-system.svc.cluster.local not karmada-aggregated-apiserver.test-karmada.svc.cluster.local, but the APIService is provided by karmada-aggregated-apiserver.test-karmada.svc.cluster.local in host cluster.

KarmadaScheduler and KarmadaKubeControllerManager will try to create lease object in namespace test-karmada not karmada-system, but namespace test-karmada doesn't exsit in Karmada.

XiShanYongYe-Chang · 2022-03-01T06:24:50Z

pkg/karmadactl/cmdinit/kubernetes/deployments.go

@@ -268,7 +269,7 @@ func (i *CommandInitOption) makeKarmadaKubeControllerManagerDeployment() *appsv1
 					"--controllers=namespace,garbagecollector,serviceaccount-token",
 					"--kubeconfig=/etc/kubeconfig",
 					"--leader-elect=true",
-					fmt.Sprintf("--leader-elect-resource-namespace=%s", i.Namespace),
+					fmt.Sprintf("--leader-elect-resource-namespace=%s", util.NamespaceKarmadaSystem),


Should this value also be configurable?

Maybe it should be. I don't know the exact scope of influence.

As far as I know, The related code is as follows:

karmada/pkg/karmadactl/cmdinit/karmada/deploy.go

Lines 42 to 49 in 0b73c6b

// create namespace

if _, err := clientSet.CoreV1().Namespaces().Create(context.TODO(), &corev1.Namespace{

ObjectMeta: metav1.ObjectMeta{

Name: namespace,

},

}, metav1.CreateOptions{}); err != nil {

klog.Exitln(err)

}

karmada/pkg/karmadactl/cmdinit/karmada/deploy.go

Lines 167 to 183 in 0b73c6b

aaService := &corev1.Service{

TypeMeta: metav1.TypeMeta{

APIVersion: "v1",

Kind: "Service",

},

ObjectMeta: metav1.ObjectMeta{

Name: "karmada-aggregated-apiserver",

Namespace: namespace,

},

Spec: corev1.ServiceSpec{

Type: corev1.ServiceTypeExternalName,

ExternalName: fmt.Sprintf("karmada-aggregated-apiserver.%s.svc.cluster.local", namespace),

},

}

if _, err := clientSet.CoreV1().Services(namespace).Create(context.TODO(), aaService, metav1.CreateOptions{}); err != nil {

return err

}

karmada/pkg/karmadactl/cmdinit/karmada/deploy.go

Lines 191 to 211 in 0b73c6b

aaAPIService := &apiregistrationv1.APIService{

TypeMeta: metav1.TypeMeta{

APIVersion: "v1",

Kind: "APIService",

},

ObjectMeta: metav1.ObjectMeta{

Name: aaAPIServiceObjName,

Labels: map[string]string{"app": "karmada-aggregated-apiserver", "apiserver": "true"},

},

Spec: apiregistrationv1.APIServiceSpec{

InsecureSkipTLSVerify: true,

Group: "cluster.karmada.io",

GroupPriorityMinimum: 2000,

Service: &apiregistrationv1.ServiceReference{

Name: "karmada-aggregated-apiserver",

Namespace: namespace,

},

Version: "v1alpha1",

VersionPriority: 10,

},

}

The above code is about hard coded namespace karmada-system in karmada which is created during karmada init.

karmada/pkg/karmadactl/cmdinit/kubernetes/deployments.go

Line 271 in 0b73c6b

fmt.Sprintf("--leader-elect-resource-namespace=%s", i.Namespace),

karmada/pkg/karmadactl/cmdinit/kubernetes/deployments.go

Line 392 in 0b73c6b

fmt.Sprintf("--leader-elect-resource-namespace=%s", i.Namespace),

The above code need the namespace which should be created in karmada during init, but now the namespace isn't created in karmada.

If the value should be configurable, maybe we can replace the hard coded namespace karmada-system？e.g.

karmada/pkg/karmadactl/cmdinit/karmada/deploy.go

Line 28 in 0b73c6b

const namespace = "karmada-system"

If you think it's OK, I can try to make the change.

I think what you're saying is quite reasonable.

These are actually two different changes, can you take them apart?

OK, then if this PR can be merged, I'll make some change base on it to replace the hard coded namespace karmada-system in karmada.

I can't get why we need to hard code the namespace here.
It's beyond the scope of this PR, right? (If yes, I recommend filing another PR for it.)

ping @sayaoailun

I can't get why we need to hard code the namespace here. It's beyond the scope of this PR, right? (If yes, I recommend filing another PR for it.)

@RainbowMango

OK, new PR after this PR.

This PR is about custom namespace in host cluster.

Next PR is about the hard coded namespace in Karmada.

What's the benefit to hard-coding the --leader-elect-resource-namespace? Or, what's the problem if we put the leader election resource under the namespace i.Namespace?

I think this PR is focusing on solving -n doesn't work issue, is it the mandatory part of the solution?

Or, what's the problem if we put the leader election resource under the namespace i.Namespace?

For now, the namespace i.Namespace was not created in Karmada but the hard coded namespace karamda-system. When i.Namespace is not set as karamda-system, KarmadaKubeControllerManager and KarmadaScheduler couldn't create the lease object.

XiShanYongYe-Chang · 2022-03-01T08:15:20Z

/cc @prodanlabs

XiShanYongYe-Chang

There are two same commit messages.

The modification LGTM.

Have you tested with another namespace except karmada-system on your local site?

sayaoailun · 2022-03-01T10:07:39Z

There are two same commit messages.

The modification LGTM.

Have you tested with another namespace except karmada-system on your local site?

Yes, I tested with another namespace.

By the way, should I merge the two commits into one?

XiShanYongYe-Chang · 2022-03-01T12:06:59Z

By the way, should I merge the two commits into one?

I think it's better.

XiShanYongYe-Chang

/lgtm

/assign @RainbowMango @prodanlabs

RainbowMango · 2022-03-04T02:08:49Z

pkg/karmadactl/cmdinit/karmada/deploy.go

@@ -28,7 +28,7 @@ import (
 const namespace = "karmada-system"

 // InitKarmadaResources Initialize karmada resource
-func InitKarmadaResources(dir, caBase64 string) error {
+func InitKarmadaResources(dir, caBase64 string, externalNamespace string) error {


Suggested change

func InitKarmadaResources(dir, caBase64 string, externalNamespace string) error {

func InitKarmadaResources(dir, caBase64 string, systemNamespace string) error {

I suggest renaming the parameter name to systemNamespace, because there is no concept of external or internal here.

karmada/pkg/karmadactl/cmdinit/karmada/deploy.go

Lines 176 to 179 in 867160a

Spec: corev1.ServiceSpec{

Type: corev1.ServiceTypeExternalName,

ExternalName: fmt.Sprintf("karmada-aggregated-apiserver.%s.svc.cluster.local", externalNamespace),

},

It's just like the external service above.

karmada/pkg/karmadactl/cmdinit/karmada/deploy.go

Lines 80 to 95 in 867160a

// create webhook configuration

// https://github.com/karmada-io/karmada/blob/master/artifacts/deploy/webhook-configuration.yaml

klog.Info("Create MutatingWebhookConfiguration mutating-config.")

if err = createMutatingWebhookConfiguration(clientSet, mutatingConfig(caBase64, externalNamespace)); err != nil {

klog.Exitln(err)

}

klog.Info("Create ValidatingWebhookConfiguration validating-config.")

if err = createValidatingWebhookConfiguration(clientSet, validatingConfig(caBase64, externalNamespace)); err != nil {

klog.Exitln(err)

}

// karmada-aggregated-apiserver

if err = initAPIService(clientSet, restConfig, externalNamespace); err != nil {

klog.Exitln(err)

}

It's the namespace out of Karmada, which is used for webhook and karmada-aggregated-apiserver in Karmada.

The external and internal are for Karmada.

I don't think so.
The namespace is used to specify the location of the service(karmada-webhook, karmada-aggregate-apiserver) which is supposed in the Karmada system namespace by default.

karmada/pkg/karmadactl/cmdinit/karmada/deploy.go

Lines 167 to 180 in 867160a

aaService := &corev1.Service{

TypeMeta: metav1.TypeMeta{

APIVersion: "v1",

Kind: "Service",

},

ObjectMeta: metav1.ObjectMeta{

Name: "karmada-aggregated-apiserver",

Namespace: namespace,

},

Spec: corev1.ServiceSpec{

Type: corev1.ServiceTypeExternalName,

ExternalName: fmt.Sprintf("karmada-aggregated-apiserver.%s.svc.cluster.local", externalNamespace),

},

}

The namespace is karmada-system in Karmada which is hard coded.

The externalNamespace is custom namespace in host cluster where Karmada is deloyed.

Yeah, I get what you mean. Once user specified the customized namespace by --namespace, like myns, which means:

Installing Karmada components to the myns namespace on host cluster. (As well as the services...)

Karmada will use the myns as the system namespace to store some configuration resources, like service/karmada-aggregated-apiserver.

Are you agree with that? Personally, I don't like to identify the namespace by internal or external as I think it brings a bit of confusion.

Karmada will use the myns as the system namespace to store some configuration resources, like service/karmada-aggregated-apiserver.

For now, Karmada will use the hard coded namespace karmada-system as the system namespace to store some configuration resources, like service/karmada-aggregated-apiserver, not myns. The resources created in karmada-system in Karmada referance the resources created in myns in host cluster.

Anyway, I'll make the change.

Yeah, you are right. Seems we have a lot of things to do.

RainbowMango · 2022-03-04T02:12:40Z

Hi @sayaoailun , Thanks for doing this.
This is the bug that we want to pick to branch release-1.0 and release-1.1, so please focus on solving the problem in this PR that will make cherry-pick more easier(less conflict).

For other optimization work, please feel free to file a new PR.

sayaoailun · 2022-03-04T03:55:35Z

Hi @sayaoailun , Thanks for doing this. This is the bug that we want to pick to branch release-1.0 and release-1.1, so please focus on solving the problem in this PR that will make cherry-pick more easier(less conflict).

For other optimization work, please feel free to file a new PR.

I checked the cherry-pick. For now, it's OK for release-1.1, but need to fix conflicts for release-1.0 which is very easy.

RainbowMango

Just one question otherwise looks good to me.

RainbowMango · 2022-03-07T01:17:35Z

pkg/karmadactl/cmdinit/kubernetes/deployments.go

@@ -268,7 +269,7 @@ func (i *CommandInitOption) makeKarmadaKubeControllerManagerDeployment() *appsv1
 					"--controllers=namespace,garbagecollector,serviceaccount-token",
 					"--kubeconfig=/etc/kubeconfig",
 					"--leader-elect=true",
-					fmt.Sprintf("--leader-elect-resource-namespace=%s", i.Namespace),
+					fmt.Sprintf("--leader-elect-resource-namespace=%s", util.NamespaceKarmadaSystem),


What's the benefit to hard-coding the --leader-elect-resource-namespace? Or, what's the problem if we put the leader election resource under the namespace i.Namespace?

I think this PR is focusing on solving -n doesn't work issue, is it the mandatory part of the solution?

RainbowMango · 2022-03-07T05:48:37Z

Hi @XiShanYongYe-Chang There is a failling test
https://github.com/karmada-io/karmada/runs/5441894841?check_suite_focus=true.

XiShanYongYe-Chang · 2022-03-07T05:53:38Z

Hi @XiShanYongYe-Chang There is a failling test https://github.com/karmada-io/karmada/runs/5441894841?check_suite_focus=true.

I will take a look.

RainbowMango · 2022-03-08T01:13:59Z

Hi @sayaoailun
I just made something stupid that I tried to rebase your code to fix the falling testing, but I mistakenly dropped your commit.

I made a copy of your commit at RainbowMango@b5a0160

Can you do it again and push it to your master branch, then re-open this PR?

sayaoailun · 2022-03-08T02:05:45Z

Hi @sayaoailun I just made something stupid that I tried to rebase your code to fix the falling testing, but I mistakenly dropped your commit.

I made a copy of your commit at RainbowMango@b5a0160

Can you do it again and push it to your master branch, then re-open this PR?

I rebased my master branch, but I don't know how to reopen this PR.

karmada-bot · 2022-03-08T02:10:21Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: RainbowMango

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~~ [RainbowMango]

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

RainbowMango · 2022-03-08T02:15:44Z

/lgtm

I rebased my master branch, but I don't know how to reopen this PR.

There is a Reopen button below. Never mind, I re-opened it and rebased your code again(to remove merge commit).

By the way, here is the PR workflow, it might be good for you.

RainbowMango · 2022-03-08T02:16:55Z

This PR will be merged automatically after testing is done.

Welcome @sayaoailun

sayaoailun · 2022-03-08T05:58:06Z

There is a Reopen button below.

Maybe I don't have permission? I didn't see it just now.

I re-opened it and rebased your code again(to remove merge commit).

@RainbowMango I wonder how this happend. My steps are as follows:

git fetch upstream
git rebase upstream/master

The merge commit seems to be 4af6414e4e891fd1416499e7f393a9fca8909b9d Merge pull request #1436 from AllenZMC/improve_test

I don't know how this merge commit is related to this PR.

I mistakenly dropped your commit.

Maybe this is the reason?

XiShanYongYe-Chang · 2022-03-08T06:27:45Z

Hi @sayaoailun, remember to pick this commit to branch release-1.0 and release-1.1.

RainbowMango · 2022-03-08T06:44:54Z

Maybe I don't have permission? I didn't see it just now.

No, we can't reopen a merged PR.

sayaoailun · 2022-03-08T07:08:27Z

Maybe I don't have permission? I didn't see it just now.

@RainbowMango I mean PR closed before PR merged.

sayaoailun · 2022-03-08T07:12:06Z

remember to pick this commit to branch release-1.0 and release-1.1.

@XiShanYongYe-Chang Done.

cherry-pick #1416: `karmadactl`: Fixed `--namespace` flag of `init` command not work issue.

karmada-bot added release-note-none Denotes a PR that doesn't merit a release note. kind/bug Categorizes issue or PR as related to a bug. labels Mar 1, 2022

karmada-bot requested review from lonelyCZ and prodanlabs March 1, 2022 03:14

karmada-bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Mar 1, 2022

XiShanYongYe-Chang reviewed Mar 1, 2022

View reviewed changes

karmada-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Mar 1, 2022

XiShanYongYe-Chang reviewed Mar 1, 2022

View reviewed changes

sayaoailun force-pushed the master branch from 6838d39 to 867160a Compare March 2, 2022 02:23

XiShanYongYe-Chang reviewed Mar 3, 2022

View reviewed changes

karmada-bot assigned prodanlabs, RainbowMango and XiShanYongYe-Chang Mar 3, 2022

karmada-bot added the lgtm Indicates that a PR is ready to be merged. label Mar 3, 2022

RainbowMango reviewed Mar 4, 2022

View reviewed changes

sayaoailun force-pushed the master branch from 867160a to b327438 Compare March 4, 2022 12:05

karmada-bot removed the lgtm Indicates that a PR is ready to be merged. label Mar 4, 2022

RainbowMango reviewed Mar 7, 2022

View reviewed changes

karmada-bot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Mar 7, 2022

RainbowMango reopened this Mar 8, 2022

karmada-bot added the do-not-merge/contains-merge-commits Indicates a PR which contains merge commits. label Mar 8, 2022

karmada-bot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Mar 8, 2022

RainbowMango force-pushed the master branch from c46e5e9 to b5a0160 Compare March 8, 2022 02:11

karmada-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed do-not-merge/contains-merge-commits Indicates a PR which contains merge commits. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 8, 2022

karmada-bot added the lgtm Indicates that a PR is ready to be merged. label Mar 8, 2022

karmada-bot merged commit 7ed9c62 into karmada-io:master Mar 8, 2022

This was referenced Mar 8, 2022

cherry-pick #1416: karmadactl: Fixed --namespace flag of init command not work issue. #1451

Merged

cherry-pick #1416: karmadactl: Fixed --namespace flag of init command not work issue. #1452

Merged

karmada-bot added a commit that referenced this pull request Mar 9, 2022

Merge pull request #1452 from sayaoailun/release-1.0

177b9ab

cherry-pick #1416: `karmadactl`: Fixed `--namespace` flag of `init` command not work issue.

karmada-bot added a commit that referenced this pull request Mar 9, 2022

Merge pull request #1451 from sayaoailun/release-1.1

231a860

cherry-pick #1416: `karmadactl`: Fixed `--namespace` flag of `init` command not work issue.

	// create namespace
	if _, err := clientSet.CoreV1().Namespaces().Create(context.TODO(), &corev1.Namespace{
	ObjectMeta: metav1.ObjectMeta{
	Name: namespace,
	},
	}, metav1.CreateOptions{}); err != nil {
	klog.Exitln(err)
	}

	aaService := &corev1.Service{
	TypeMeta: metav1.TypeMeta{
	APIVersion: "v1",
	Kind: "Service",
	},
	ObjectMeta: metav1.ObjectMeta{
	Name: "karmada-aggregated-apiserver",
	Namespace: namespace,
	},
	Spec: corev1.ServiceSpec{
	Type: corev1.ServiceTypeExternalName,
	ExternalName: fmt.Sprintf("karmada-aggregated-apiserver.%s.svc.cluster.local", namespace),
	},
	}
	if _, err := clientSet.CoreV1().Services(namespace).Create(context.TODO(), aaService, metav1.CreateOptions{}); err != nil {
	return err
	}

	aaAPIService := &apiregistrationv1.APIService{
	TypeMeta: metav1.TypeMeta{
	APIVersion: "v1",
	Kind: "APIService",
	},
	ObjectMeta: metav1.ObjectMeta{
	Name: aaAPIServiceObjName,
	Labels: map[string]string{"app": "karmada-aggregated-apiserver", "apiserver": "true"},
	},
	Spec: apiregistrationv1.APIServiceSpec{
	InsecureSkipTLSVerify: true,
	Group: "cluster.karmada.io",
	GroupPriorityMinimum: 2000,
	Service: &apiregistrationv1.ServiceReference{
	Name: "karmada-aggregated-apiserver",
	Namespace: namespace,
	},
	Version: "v1alpha1",
	VersionPriority: 10,
	},
	}

	func InitKarmadaResources(dir, caBase64 string, externalNamespace string) error {
	func InitKarmadaResources(dir, caBase64 string, systemNamespace string) error {

	// create webhook configuration
	// https://github.com/karmada-io/karmada/blob/master/artifacts/deploy/webhook-configuration.yaml
	klog.Info("Create MutatingWebhookConfiguration mutating-config.")
	if err = createMutatingWebhookConfiguration(clientSet, mutatingConfig(caBase64, externalNamespace)); err != nil {
	klog.Exitln(err)
	}
	klog.Info("Create ValidatingWebhookConfiguration validating-config.")

	if err = createValidatingWebhookConfiguration(clientSet, validatingConfig(caBase64, externalNamespace)); err != nil {
	klog.Exitln(err)
	}

	// karmada-aggregated-apiserver
	if err = initAPIService(clientSet, restConfig, externalNamespace); err != nil {
	klog.Exitln(err)
	}

karmadactl: Fixed --namespace flag of init command not work issue. #1416

karmadactl: Fixed --namespace flag of init command not work issue. #1416

Conversation

sayaoailun commented Mar 1, 2022 • edited by RainbowMango

karmada-bot commented Mar 1, 2022

RainbowMango commented Mar 1, 2022

sayaoailun commented Mar 1, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sayaoailun Mar 4, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

XiShanYongYe-Chang commented Mar 1, 2022

XiShanYongYe-Chang left a comment

Choose a reason for hiding this comment

sayaoailun commented Mar 1, 2022

XiShanYongYe-Chang commented Mar 1, 2022

XiShanYongYe-Chang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sayaoailun Mar 4, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RainbowMango commented Mar 4, 2022

sayaoailun commented Mar 4, 2022

RainbowMango left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RainbowMango commented Mar 7, 2022

XiShanYongYe-Chang commented Mar 7, 2022

RainbowMango commented Mar 8, 2022

sayaoailun commented Mar 8, 2022

karmada-bot commented Mar 8, 2022

RainbowMango commented Mar 8, 2022

RainbowMango commented Mar 8, 2022

sayaoailun commented Mar 8, 2022

XiShanYongYe-Chang commented Mar 8, 2022

RainbowMango commented Mar 8, 2022

sayaoailun commented Mar 8, 2022 • edited

sayaoailun commented Mar 8, 2022

`karmadactl`: Fixed `--namespace` flag of `init` command not work issue. #1416

`karmadactl`: Fixed `--namespace` flag of `init` command not work issue. #1416

sayaoailun commented Mar 1, 2022 •

edited by RainbowMango

sayaoailun commented Mar 1, 2022 •

edited

sayaoailun Mar 4, 2022 •

edited

sayaoailun Mar 4, 2022 •

edited

sayaoailun commented Mar 8, 2022 •

edited