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
kubeadm: Fully implement --dry-run #51122
Conversation
cmd/kubeadm/app/cmd/init.go
Outdated
@@ -374,3 +387,37 @@ func createClientsetAndOptionallyWaitForReady(cfg *kubeadmapi.MasterConfiguratio | |||
} | |||
return client, nil | |||
} | |||
|
|||
func getDirectoriesToUse(dryRun bool, defaultPkiDir string) (string, string, string, error) { | |||
|
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.
Remove blank line please
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.
done
cmd/kubeadm/app/cmd/init.go
Outdated
if dryRun { | ||
dryRunDir, err := ioutil.TempDir("", "kubeadm-init-dryrun") | ||
if err != nil { | ||
return "", "", "", fmt.Errorf("couldn't create a temporary directory for the upgrade: %v") |
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.
Is this always for an upgrade?
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.
nope, oops. Copy-paste error :)
cmd/kubeadm/app/cmd/init.go
Outdated
@@ -374,3 +387,37 @@ func createClientsetAndOptionallyWaitForReady(cfg *kubeadmapi.MasterConfiguratio | |||
} | |||
return client, nil | |||
} | |||
|
|||
func getDirectoriesToUse(dryRun bool, defaultPkiDir string) (string, string, string, error) { |
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 would like to find an alternative to a bool
param but I don't have any great ideas (they're unintuitive).
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 hope we can defer that until after code freeze. Agree that it would be nice to refactor this once we have the full picture afterwards.
cmd/kubeadm/app/cmd/init.go
Outdated
return defaultPkiDir, kubeadmconstants.KubernetesDir, kubeadmconstants.GetStaticPodDirectory(), nil | ||
} | ||
|
||
func printFilesIfDryRunning(dryRun bool, manifestDir string) error { |
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.
Maybe don't take the bool in here, and do the if check where you call it?
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.
deferring that to later
cmd/kubeadm/app/cmd/init.go
Outdated
|
||
fmt.Printf("[dryrun] Wrote certificates, kubeconfig files and control plane manifests to %q\n", manifestDir) | ||
fmt.Println("[dryrun] Won't print certificates or kubeconfig files due to the sensitive nature of them") | ||
fmt.Println("[dryrun] Please go an examine the %q directory for details about what would be written\n", manifestDir) |
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.
Printf instead of Println.
Either remove "an" or change it to "and".
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.
goh, should be Printf
and and
, thanks for catching
b, err := yaml.Marshal(obj) | ||
return b, err | ||
func DefaultMarshalFunc(obj runtime.Object, gv schema.GroupVersion) ([]byte, error) { | ||
|
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.
Remove
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.
done
// DryRunPrintFileContents | ||
func DryRunPrintFileContents(files []DryRunFile, w io.Writer) error { | ||
for _, file := range files { | ||
|
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.
Remove
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.
done
|
||
fileBytes, err := ioutil.ReadFile(file.RealPath) | ||
if err != nil { | ||
return err |
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.
Do you want to stop on first error, or continue and try to do as many as you can?
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.
good idea, incorporated
29c3b61
to
a850410
Compare
@ncdc Thanks for the review! Should now be fixed up and ready to merge 👍 |
fmt.Fprintf(w, "[dryrun] Would write file %q with content:\n", outputFilePath) | ||
PrintBytesWithLinePrefix(w, fileBytes, "\t") | ||
} | ||
if len(errors) != 0 { |
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.
You might just want to return k8s.io/apimachinery/pkg/util/errors.NewAggregate(errors)
, which will do the right thing if len(errors) is 0.
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.
fixed
c247d90
to
5d22be6
Compare
@ncdc fixed the nit; now things should be green :) |
I reviewed for anything that stood out to me, but as I'm not very familiar with this code, I think @timothysc should review too. |
Please make this not say |
Also |
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.
Could you outline why you decided to switch from a central config to passing params around, I actually liked it better b4.
@@ -242,28 +244,40 @@ func (i *Init) Run(out io.Writer) error { | |||
return fmt.Errorf("couldn't parse kubernetes version %q: %v", i.cfg.KubernetesVersion, err) | |||
} | |||
|
|||
// Get directories to write files to; can be faked if we're dry-running |
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'm still kind of flummoxed why we are passing args around vs. having a centralized configuration object. For example, for testing and validation I may want to dump the state of config during a dryRun for *, which would include the tempdirs b/c I want to inspect the certs.
|
||
saSigningKey, err := NewServiceAccountSigningKey() | ||
if err != nil { | ||
return err | ||
} | ||
|
||
return writeKeyFilesIfNotExist( | ||
cfg.CertificatesDir, | ||
pkiDir, |
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 fail to see why this is better?
5d22be6
to
037f485
Compare
037f485
to
236c068
Compare
/retest |
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.
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: luxas, timothysc Associated issue: 50631 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
236c068
to
2c6c748
Compare
Rebased to get it into the queue |
/retest |
2c6c748
to
2c71814
Compare
rebased again |
/retest |
Automatic merge from submit-queue (batch tested with PRs 51134, 51122, 50562, 50971, 51327) |
Judging by the title |
@f0o hehe, right, this was "fully implement" for |
What this PR does / why we need it:
Finishes the work begun in #50631
apiVersion
andkind
wasn't printed earlier.Which issue this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close that issue when PR gets merged): fixes #fixes: kubernetes/kubeadm#389
Special notes for your reviewer:
Full
kubeadm init --dry-run
output:Release note:
@kubernetes/sig-cluster-lifecycle-pr-reviews @ncdc @sttts