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

Remove the dependency between create priorityclass command and genera… #94893

Merged
merged 1 commit into from Nov 5, 2020
Merged

Remove the dependency between create priorityclass command and genera… #94893

merged 1 commit into from Nov 5, 2020

Conversation

aubm
Copy link
Contributor

@aubm aubm commented Sep 18, 2020 •
edited

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

Pursuing the work started in #90676, by removing the generators dependency from the kubectl create clusterrolebinding command.

Which issue(s) this PR fixes:

Partially #93100

@k8s-ci-robot k8s-ci-robot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Sep 18, 2020
@k8s-ci-robot
Copy link
Contributor

Hi @aubm. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Sep 18, 2020
@aubm
Copy link
Contributor Author

aubm commented Sep 18, 2020

/assign soltysh

@k8s-ci-robot k8s-ci-robot added area/kubectl sig/cli Categorizes an issue or PR as relevant to SIG CLI. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Sep 18, 2020
@aubm
Copy link
Contributor Author

aubm commented Sep 18, 2020

/assign @liggitt

@aubm
Copy link
Contributor Author

aubm commented Sep 18, 2020

/assign @zhouya0

@liggitt
Copy link
Member

liggitt commented Sep 18, 2020

@soltysh seems to have more context on this

/unassign
/assign @soltysh

@brianpursley
Copy link
Member

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 24, 2020
@aubm
Copy link
Contributor Author

aubm commented Sep 24, 2020

/retest

3 similar comments
@aubm
Copy link
Contributor Author

aubm commented Sep 24, 2020

/retest

@aubm
Copy link
Contributor Author

aubm commented Sep 24, 2020

/retest

@aubm
Copy link
Contributor Author

aubm commented Sep 27, 2020

/retest

soltysh
soltysh approved these changes Nov 2, 2020
View reviewed changes
Copy link
Contributor

@soltysh soltysh left a comment

Choose a reason for hiding this comment

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

One nit.
/lgtm
/approve
/retest

staging/src/k8s.io/kubectl/pkg/cmd/create/create_priorityclass.go
func NewPriorityClassOptions(ioStreams genericclioptions.IOStreams) *PriorityClassOptions {
return &PriorityClassOptions{
Value: 0,
PreemptionPolicy: "PreemptLowerPriority",
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe worth using corev1.PreemptLowerPriority to ensure there's no typo.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 2, 2020
@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 2, 2020
@soltysh
Copy link
Contributor

soltysh commented Nov 2, 2020

/priority backlog
/release-note none

@k8s-ci-robot k8s-ci-robot added priority/backlog Higher priority than priority/awaiting-more-evidence. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Nov 2, 2020
@soltysh soltysh added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Nov 2, 2020
@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

5 similar comments
@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@soltysh
Copy link
Contributor

soltysh commented Nov 3, 2020

/hold
/lgtm cancel
@aubm looks like the integration failure is real

@k8s-ci-robot k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Nov 3, 2020
@aubm
Copy link
Contributor Author

aubm commented Nov 3, 2020

@soltysh I'll try and take a look by the end of the week, but I find the errors logs to be quite misleading to be honest.

warning: Immediate deletion does not wait for confirmation that the running resource has been terminated. The resource may continue to run on the cluster indefinitely.
warning: Immediate deletion does not wait for confirmation that the running resource has been terminated. The resource may continue to run on the cluster indefinitely.
warning: Immediate deletion does not wait for confirmation that the running resource has been terminated. The resource may continue to run on the cluster indefinitely.
error: resource(s) were provided, but no name, label selector, or --all flag specified
error: setting 'all' parameter but found a non empty selector. 
warning: Immediate deletion does not wait for confirmation that the running resource has been terminated. The resource may continue to run on the cluster indefinitely.
error: min-available and max-unavailable cannot be both specified
Warning: apiextensions.k8s.io/v1beta1 CustomResourceDefinition is deprecated in v1.16+, unavailable in v1.22+; use apiextensions.k8s.io/v1 CustomResourceDefinition
error: /, Kind= doesn't support dry-run
!!! [1103 11:20:22] Call tree:
!!! [1103 11:20:22]  1: /home/prow/go/src/k8s.io/kubernetes/test/cmd/../../third_party/forked/shell2junit/sh2ju.sh:47 run_pod_tests(...)
!!! [1103 11:20:22]  2: /home/prow/go/src/k8s.io/kubernetes/test/cmd/../../third_party/forked/shell2junit/sh2ju.sh:112 eVal(...)
!!! [1103 11:20:22]  3: /home/prow/go/src/k8s.io/kubernetes/test/cmd/legacy-script.sh:131 juLog(...)
!!! [1103 11:20:22]  4: /home/prow/go/src/k8s.io/kubernetes/test/cmd/legacy-script.sh:507 record_command(...)
!!! [1103 11:20:22]  5: hack/make-rules/test-cmd.sh:184 runTests(...)

If I isolate the three error lines, three entries appear and don't seem to be related.

error: resource(s) were provided, but no name, label selector, or --all flag specified
error: setting 'all' parameter but found a non empty selector. 
error: /, Kind= doesn't support dry-run

Am I reading this right? Any tips on the path for investigation.

soltysh
soltysh requested changes Nov 4, 2020
View reviewed changes
Copy link
Contributor

@soltysh soltysh left a comment

Choose a reason for hiding this comment

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

/lgtm cancel

staging/src/k8s.io/kubectl/pkg/cmd/create/create_priorityclass.go Outdated
"github.com/spf13/cobra"

apiv1 "k8s.io/api/core/v1"
corev1 "k8s.io/api/core/v1"
scheduling "k8s.io/api/scheduling/v1"
Copy link
Contributor

Choose a reason for hiding this comment

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

schedulingv1 please to keep convention 😉

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done 🙂

staging/src/k8s.io/kubectl/pkg/cmd/create/create_priorityclass.go Outdated
"k8s.io/cli-runtime/pkg/genericclioptions"
"k8s.io/cli-runtime/pkg/resource"
schedulingclient "k8s.io/client-go/kubernetes/typed/scheduling/v1"
Copy link
Contributor

Choose a reason for hiding this comment

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

schedulingv1client

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done 🙂

staging/src/k8s.io/kubectl/pkg/cmd/create/create_priorityclass.go
func (o *PriorityClassOptions) createPriorityClass() (*scheduling.PriorityClass, error) {
preemptionPolicy := corev1.PreemptionPolicy(o.PreemptionPolicy)
return &scheduling.PriorityClass{
ObjectMeta: metav1.ObjectMeta{
Copy link
Contributor

Choose a reason for hiding this comment

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

You're missing:

		// this is ok because we know exactly how we want to be serialized
		TypeMeta: metav1.TypeMeta{APIVersion: scheduling.SchemeGroupVersion.String(), Kind: "PriorityClass"},

here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done et rebased on master 🙂

staging/src/k8s.io/kubectl/pkg/cmd/create/create_priorityclass.go
createOptions.FieldManager = o.FieldManager
}
if o.DryRunStrategy == cmdutil.DryRunServer {
if err := o.DryRunVerifier.HasSupport(priorityClass.GroupVersionKind()); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is having problems b/c you miss setting that TypeMeta below, which ends up kubectl asking the server for support for non-existent GVK. When you add that, you'll notice that these two lines are passing

kubernetes/test/cmd/core.sh

Lines 292 to 293 in 396b90f

kubectl create priorityclass test-priorityclass --dry-run=client
kubectl create priorityclass test-priorityclass --dry-run=server

It's best to look through logs looking for last logs with line numbers and try to figure out which might have failed. In your case you'll notice:

core.sh:290: Successful get priorityclasses {{range.items}}{{ if eq .metadata.name \"test-priorityclass\" }}found{{end}}{{end}}:: :

so the next lines of actual code are those create lines I've pointed out earlier. Running locally both will reveal the problem is with the next and from here it's pretty easy to nail down the actual code issue ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great! Thank you for taking the time to walk me through it @soltysh 🙂
Next time I should be more comfortable.

@k8s-ci-robot k8s-ci-robot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 4, 2020
@aubm aubm force-pushed the refactor-command-kubectl-create-priorityclass branch from dfcc441 to 9fff7f7 Compare November 5, 2020 14:34
soltysh
soltysh approved these changes Nov 5, 2020
View reviewed changes
Copy link
Contributor

@soltysh soltysh left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@soltysh
Copy link
Contributor

soltysh commented Nov 5, 2020

/hold cancel

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Nov 5, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aubm, soltysh

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 5, 2020
@soltysh
Copy link
Contributor

soltysh commented Nov 5, 2020

/retest

1 similar comment
@aubm
Copy link
Contributor Author

aubm commented Nov 5, 2020

/retest

@k8s-ci-robot k8s-ci-robot merged commit 544f42e into kubernetes:master Nov 5, 2020
@k8s-ci-robot k8s-ci-robot added this to the v1.20 milestone Nov 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@soltysh soltysh soltysh approved these changes

@brianpursley brianpursley Awaiting requested review from brianpursley

@mengqiy mengqiy Awaiting requested review from mengqiy

Assignees

@soltysh soltysh

@zhouya0 zhouya0

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/kubectl cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. priority/backlog Higher priority than priority/awaiting-more-evidence. release-note-none Denotes a PR that doesn't merit a release note. sig/cli Categorizes an issue or PR as relevant to SIG CLI. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v1.20
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@aubm @k8s-ci-robot @liggitt @brianpursley @soltysh @fejta-bot @zhouya0