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

kubeadm: reduce the number of fetch configuration when reset #74410

Merged
merged 1 commit into from
Feb 26, 2019
Merged

kubeadm: reduce the number of fetch configuration when reset #74410

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions cmd/kubeadm/app/cmd/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,7 @@ go_test(
],
embed = [":go_default_library"],
deps = [
"//cmd/kubeadm/app/apis/kubeadm:go_default_library",
"//cmd/kubeadm/app/apis/kubeadm/v1beta1:go_default_library",
"//cmd/kubeadm/app/apis/kubeadm/validation:go_default_library",
"//cmd/kubeadm/app/cmd/options:go_default_library",
Expand Down
50 changes: 22 additions & 28 deletions cmd/kubeadm/app/cmd/reset.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ import (
"k8s.io/apimachinery/pkg/util/sets"
clientset "k8s.io/client-go/kubernetes"
"k8s.io/klog"
kubeadmapi "k8s.io/kubernetes/cmd/kubeadm/app/apis/kubeadm"
kubeadmapiv1beta1 "k8s.io/kubernetes/cmd/kubeadm/app/apis/kubeadm/v1beta1"
"k8s.io/kubernetes/cmd/kubeadm/app/apis/kubeadm/validation"
"k8s.io/kubernetes/cmd/kubeadm/app/cmd/options"
Expand Down Expand Up @@ -68,15 +69,20 @@ func NewCmdReset(in io.Reader, out io.Writer) *cobra.Command {
kubeadmutil.CheckErr(err)
}

cfg, err := configutil.FetchInitConfigurationFromCluster(client, os.Stdout, "reset", false)
if err != nil {
klog.Warningf("[reset] Unable to fetch the kubeadm-config ConfigMap from cluster: %v", err)
}

if criSocketPath == "" {
Copy link
Member

Choose a reason for hiding this comment

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

we need to check if cfg is not nil here.

criSocketPath, err = resetDetectCRISocket(client)
criSocketPath, err = resetDetectCRISocket(cfg)
kubeadmutil.CheckErr(err)
klog.V(1).Infof("[reset] detected and using CRI socket: %s", criSocketPath)
}

r, err := NewReset(in, ignorePreflightErrorsSet, forceReset, certsDir, criSocketPath)
kubeadmutil.CheckErr(err)
kubeadmutil.CheckErr(r.Run(out, client))
kubeadmutil.CheckErr(r.Run(out, client, cfg))
Copy link
Member

Choose a reason for hiding this comment

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

i don't think we should call Run if cfg is nil

Copy link
Contributor Author

@pytimer pytimer Feb 26, 2019 •
edited

Choose a reason for hiding this comment

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

We not call Run if cfg is nil, reset will be aborted.
I see that in the Run workflow, the next processes will continue, even though have the mistakes of the previous step.
I am not sure if this place the same as Run workflow.

Copy link
Member

Choose a reason for hiding this comment

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

yes, i guess that's true.
i guess we can pass nil here but inside Run we need to not use cfg.SomeField because it will panic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I check the currently code, I find resetDetectCRISocket , getEtcdDataDir and RemoveStackedEtcdMemberFromCluster using cfg, and we should check cfg is not nil in these place if we pass nil here.
Should i check cfg is nil before invoke Run, or check it inside Run? Which one do you prefer?

Copy link
Member

Choose a reason for hiding this comment

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

so we should run all the code that doesn't depend on cfg, but if a line is calling cfg.SomeField we should always check if cfg is nil before that.

Copy link
Contributor Author

@pytimer pytimer Feb 26, 2019 •
edited

Choose a reason for hiding this comment

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

yes, you said right.

I considered it, maybe better to check cfg in advance, it is similar to invoke getClientset check client variable.

I update the code in my local development environment, below is code:

cfg, err := configutil.FetchInitConfigurationFromCluster(client, os.Stdout, "reset", false)
if err != nil {
	kubeadmutil.CheckErr(errors.Errorf("[reset] Unable to fetch the kubeadm-config ConfigMap from cluster: %v\n", err))
}

If we check cfg is nil , reset will be aborted. If user wants to continue cleanup, should we tell them how to do it?

Copy link
Member

Choose a reason for hiding this comment

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

replied here: https://github.com/kubernetes/kubernetes/pull/74410/files#r260095993

},
}

Expand Down Expand Up @@ -131,17 +137,19 @@ func NewReset(in io.Reader, ignorePreflightErrors sets.String, forceReset bool,
}

// Run reverts any changes made to this host by "kubeadm init" or "kubeadm join".
func (r *Reset) Run(out io.Writer, client clientset.Interface) error {
func (r *Reset) Run(out io.Writer, client clientset.Interface, cfg *kubeadmapi.InitConfiguration) error {
var dirsToClean []string
// Only clear etcd data when using local etcd.
etcdManifestPath := filepath.Join(kubeadmconstants.KubernetesDir, kubeadmconstants.ManifestsSubDirName, "etcd.yaml")

klog.V(1).Infof("[reset] checking for etcd config")
etcdDataDir, err := getEtcdDataDir(etcdManifestPath, client)
etcdDataDir, err := getEtcdDataDir(etcdManifestPath, cfg)
if err == nil {
dirsToClean = append(dirsToClean, etcdDataDir)
if err := removeEtcdMember(client); err != nil {
klog.Warningf("[reset] failed to remove etcd member: %v\n.Please manually remove this etcd member using etcdctl", err)
if cfg != nil {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Add check. Because etcdphase.RemoveStackedEtcdMemberFromCluster(client, cfg) function will use cfg and not check it.

Copy link
Member

Choose a reason for hiding this comment

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

nice catch.

if err := etcdphase.RemoveStackedEtcdMemberFromCluster(client, cfg); err != nil {
klog.Warningf("[reset] failed to remove etcd member: %v\n.Please manually remove this etcd member using etcdctl", err)
}
}
} else {
fmt.Println("[reset] no etcd config found. Assuming external etcd")
Expand Down Expand Up @@ -209,25 +217,14 @@ func (r *Reset) Run(out io.Writer, client clientset.Interface) error {
return nil
}

func removeEtcdMember(client clientset.Interface) error {
cfg, err := configutil.FetchInitConfigurationFromCluster(client, os.Stdout, "reset", false)
if err != nil {
return err
}
return etcdphase.RemoveStackedEtcdMemberFromCluster(client, cfg)
}

func getEtcdDataDir(manifestPath string, client clientset.Interface) (string, error) {
func getEtcdDataDir(manifestPath string, cfg *kubeadmapi.InitConfiguration) (string, error) {
const etcdVolumeName = "etcd-data"
var dataDir string

if client != nil {
cfg, err := configutil.FetchInitConfigurationFromCluster(client, os.Stdout, "reset", false)
if err == nil && cfg.Etcd.Local != nil {
return cfg.Etcd.Local.DataDir, nil
}
klog.Warningf("[reset] Unable to fetch the kubeadm-config ConfigMap, using etcd pod spec as fallback: %v", err)
if cfg != nil && cfg.Etcd.Local != nil {
return cfg.Etcd.Local.DataDir, nil
}
klog.Warningln("[reset] No kubeadm config, using etcd pod spec to get data directory")

etcdPod, err := utilstaticpod.ReadStaticPodFromDisk(manifestPath)
if err != nil {
Expand Down Expand Up @@ -311,13 +308,10 @@ func resetConfigDir(configPathDir, pkiPathDir string) {
}
}

func resetDetectCRISocket(client clientset.Interface) (string, error) {
if client != nil {
// first try to connect to the cluster for the CRI socket
cfg, err := configutil.FetchInitConfigurationFromCluster(client, os.Stdout, "reset", false)
if err == nil {
return cfg.NodeRegistration.CRISocket, nil
}
func resetDetectCRISocket(cfg *kubeadmapi.InitConfiguration) (string, error) {
if cfg != nil {
// first try to get the CRI socket from the cluster configuration
return cfg.NodeRegistration.CRISocket, nil
}

// if this fails, try to detect it
Expand Down
20 changes: 10 additions & 10 deletions cmd/kubeadm/app/cmd/reset_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ import (

"github.com/lithammer/dedent"

clientsetfake "k8s.io/client-go/kubernetes/fake"
kubeadmapi "k8s.io/kubernetes/cmd/kubeadm/app/apis/kubeadm"
kubeadmapiv1beta1 "k8s.io/kubernetes/cmd/kubeadm/app/apis/kubeadm/v1beta1"
"k8s.io/kubernetes/cmd/kubeadm/app/apis/kubeadm/validation"
kubeadmconstants "k8s.io/kubernetes/cmd/kubeadm/app/constants"
Expand Down Expand Up @@ -275,38 +275,38 @@ func TestGetEtcdDataDir(t *testing.T) {
podYaml string
expectErr bool
writeManifest bool
validClient bool
validConfig bool
}{
"non-existent file returns error": {
expectErr: true,
writeManifest: false,
validClient: true,
validConfig: true,
},
"return etcd data dir": {
dataDir: "/path/to/etcd",
podYaml: etcdPod,
expectErr: false,
writeManifest: true,
validClient: true,
validConfig: true,
},
"invalid etcd pod": {
podYaml: etcdPodInvalid,
expectErr: true,
writeManifest: true,
validClient: true,
validConfig: true,
},
"etcd pod spec without data volume": {
podYaml: etcdPodWithoutDataVolume,
expectErr: true,
writeManifest: true,
validClient: true,
validConfig: true,
},
"kubeconfig file doesn't exist": {
dataDir: "/path/to/etcd",
podYaml: etcdPod,
expectErr: false,
writeManifest: true,
validClient: false,
validConfig: false,
},
}

Expand All @@ -325,9 +325,9 @@ func TestGetEtcdDataDir(t *testing.T) {

var dataDir string
var err error
if test.validClient {
client := clientsetfake.NewSimpleClientset()
dataDir, err = getEtcdDataDir(manifestPath, client)
if test.validConfig {
cfg := &kubeadmapi.InitConfiguration{}
dataDir, err = getEtcdDataDir(manifestPath, cfg)
} else {
dataDir, err = getEtcdDataDir(manifestPath, nil)
}
Expand Down