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.

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

Node selector is a dict #2733

Merged
merged 4 commits into from Nov 5, 2019
Merged

Node selector is a dict #2733

merged 4 commits into from Nov 5, 2019

Conversation

@The-Loeki
Copy link
Contributor

The-Loeki commented Oct 17, 2019

This PR is a

  • Feature Implementation
  • Bug Fix

What this PR does / why we need it:
The nodeSelector parameters in the Helm chart are forced 'this: is not a string'
This PR makes it dicts, improving usability and consistency

Please leave this checklist in the PR comment so that maintainers can ensure a good PR.

Merge Checklist:

  • Server Flag for config
    • Chart changes
    • removing a flag by marking deprecated and hiding to avoid
      breaking the chart release and existing clients who provide a
      flag that will get an error when they try to update
The-Loeki added 2 commits Oct 17, 2019
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Oct 17, 2019

Welcome @The-Loeki!

It looks like this is your first PR to kubernetes-sigs/service-catalog 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/service-catalog has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Oct 17, 2019

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.


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. I understand the commands that are listed here.

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Oct 17, 2019

Hi @The-Loeki. Thanks for your PR.

I'm waiting for a kubernetes-sigs or 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 requested review from MHBauer and piotrmiskiewicz Oct 17, 2019
@mszostok

This comment has been minimized.

Copy link
Member

mszostok commented Oct 17, 2019

Hi @The-Loeki

Thanks for your PR:) please sign the CLA ;) #2733 (comment)

/ok-to-test

The-Loeki and others added 2 commits Oct 17, 2019
Co-Authored-By: Mateusz Szostok <szostok.mateusz@gmail.com>
Co-Authored-By: Mateusz Szostok <szostok.mateusz@gmail.com>
@The-Loeki

This comment has been minimized.

Copy link
Contributor Author

The-Loeki commented Oct 17, 2019

Hi @mszostok, thanks for the quick response!

I'm already walking through the CLA process, so that's fixxed shortly.

Thing is, sorry about the 'stupid-and-quick' haxxor, but I can't seem to git clone/checkout because there's a bunch of paths in the repo that are apparently too long... So I can't really process/test my changes here :/

Also, I was wondering; principal reason to do this for us is to force the stuff onto Linux kubelets. I can't seem to figure out why the Jobs seem not to need it though, they keep running on Linux kubelets even though there's no taints or anything involved. As this is a test-cluster this could be 'coincidence', but maybe I should add the same nodeselector to the jobs too?

@mszostok

This comment has been minimized.

Copy link
Member

mszostok commented Oct 17, 2019

Right now we have 3 jobs, they are short-lived and the nodeSelector was applied only for the main deployment of the Service Catalog: controller-manager and webhook-server.

If you have a case that all Pods (even the short-lived e.g. from migration) related to the Service Catalog should be scheduled to the given node then on this PR you can specify in values.yaml:

jobs:
  # nodeSelector to apply to pods from all Jobs which run to completion, e.g. cleaner, migration etc.
  nodeSelector:  

and add for each job:

    {{ if .Values.controllerManager.nodeSelector }}
      nodeSelector:
{{ toYaml .Values.jobs.nodeSelector | indent 8 }}
    {{ end }}

so it will be configurable for you:)

@mszostok

This comment has been minimized.

Copy link
Member

mszostok commented Oct 17, 2019

tbh I do not get that, point:

(..) there's a bunch of paths in the repo that are apparently too long...

what do you mean by that? you can execute just

git clone git@github.com:kubernetes-sigs/service-catalog.git $GOPATH/src/github.com/kubernetes-sigs/service-catalog

and it should work

@The-Loeki

This comment has been minimized.

Copy link
Contributor Author

The-Loeki commented Oct 17, 2019

you can execute just

git clone git@github.com:kubernetes-sigs/service-catalog.git $GOPATH/src/github.com/kubernetes-sigs/service-catalog

and it should work

[theloeki@murphy upstream]$ git clone git@github.com:kubernetes-sigs/service-catalog.git
Cloning into 'service-catalog'...
remote: Enumerating objects: 21, done.
remote: Counting objects: 100% (21/21), done.
remote: Compressing objects: 100% (21/21), done.
remote: Total 73652 (delta 5), reused 5 (delta 0), pack-reused 73631
Receiving objects: 100% (73652/73652), 106.18 MiB | 6.20 MiB/s, done.
Resolving deltas: 100% (37181/37181), done.
error: unable to create file cmd/svcat/testdata/responses/catalog/clusterserviceplans_labelSelector=servicecatalog.k8s.io_spec.clusterServiceClassRef.name=4f6e6cf6-ffdd-425f-a2c7-3c9258ad2468,servicecatalog.k8s.io_spec.externalName=default.json: File name too long
error: unable to create file cmd/svcat/testdata/responses/catalog/clusterserviceplans_labelSelector=servicecatalog.k8s.io_spec.externalName=default,servicecatalog.k8s.io_spec.serviceClassRef.name=4f6e6cf6-ffdd-425f-a2c7-3c9258ad2468.json: File name too long
error: unable to create file cmd/svcat/testdata/responses/catalog/namespaces/default/serviceplans_labelSelector=servicecatalog.k8s.io_spec.externalName=namespacedplan,servicecatalog.k8s.io_spec.serviceClassRef.name=4f6e6cf6-ffdd-425f-a2c7-3c9258ad2468.json: File name too long
error: unable to create file cmd/svcat/testdata/responses/catalog/namespaces/test-ns/serviceplans_labelSelector=servicecatalog.k8s.io_spec.clusterServiceClassRef.name=4f6e6cf6-ffdd-425f-a2c7-3c9258ad2468,servicecatalog.k8s.io_spec.externalName=default.jso: File name too long
error: unable to create file cmd/svcat/testdata/responses/catalog/namespaces/test-ns/serviceplans_labelSelector=servicecatalog.k8s.io_spec.externalName=default,servicecatalog.k8s.io_spec.serviceClassRef.name=4f6e6cf6-ffdd-425f-a2c7-3c9258ad2468.json: File name too long
Checking out files: 100% (4889/4889), done.
fatal: unable to checkout working tree
warning: Clone succeeded, but checkout failed.
You can inspect what was checked out with 'git status'
and retry the checkout with 'git checkout -f HEAD'

[theloeki@murphy upstream]$ cd service-catalog/ ; git status
On branch master
Your branch is up to date with 'origin/master'.

Changes to be committed:
  (use "git reset HEAD <file>..." to unstage)

        deleted:    .dockerignore
        deleted:    .github/ISSUE_TEMPLATE.md
        deleted:    .github/PULL_REQUEST_TEMPLATE.md
        deleted:    .gitignore
        deleted:    .travis.yml
       <<SNIP>>
Untracked files:
  (use "git add <file>..." to include in what will be committed)

        .dockerignore
        .github/
        .gitignore
        .travis.yml
        CONTRIBUTING.md
        Gopkg.lock
        Gopkg.toml
        Jenkinsfile
        LICENSE
        Makefile
        OWNERS
        README.md
        REVIEWING.md
        SECURITY_CONTACTS
        build/
        charts/
        cmd/
        code-of-conduct.md
        contrib/
        docs/
        docsite/
        internal/
        netlify.toml
        pkg/
        plugin/
        terminology.md
        test/
        vendor/
@mszostok

This comment has been minimized.

Copy link
Member

mszostok commented Oct 17, 2019

what is your OS? I'm working from macOS Mojave, version 10.14.6 and I didn't have any problem

but maybe this will help you: https://stackoverflow.com/a/22575737

I will also try to consider what to do with such file names.

@mszostok

This comment has been minimized.

Copy link
Member

mszostok commented Oct 22, 2019

Hi @The-Loeki, any updates about this proposed solution: #2733 (comment)

is that ok for you? Should I add missing things or you will do that?

@The-Loeki

This comment has been minimized.

Copy link
Contributor Author

The-Loeki commented Oct 23, 2019

Hi @mszostok sorry about that, this one fell a bit off my radar for now ;)

The fix didn't work (I'm on Fedora 30 btw, which somehow exhibits this behaviour without being Windows; I've Googled around a little bit and found mostly the same fix as you proposed)...

I did however roll out the SC a few more times this week, and still never ran into a dying job for not being on a Linux machine, so I'm guessing it's not really necessary to add those annotations just yet, although I still can't think why; if they're not there, I'd expect the scheduler to be fully free to schedule a job just like any other app (and as the Windows node sits near-empty for now that would've been a prime candidate in any way), so I'm still a bit confused by that :)

@The-Loeki

This comment has been minimized.

Copy link
Contributor Author

The-Loeki commented Oct 23, 2019

[theloeki@murphy upstream]$ git clone https://github.com/kubernetes-sigs/service-catalog.git
Cloning into 'service-catalog'...
remote: Enumerating objects: 73652, done.
remote: Total 73652 (delta 0), reused 0 (delta 0), pack-reused 73652
Receiving objects: 100% (73652/73652), 106.18 MiB | 6.64 MiB/s, done.
Resolving deltas: 100% (37191/37191), done.
Checking out files: 100% (4889/4889), done.

Temporary insanity, that's what it was...

@mszostok

This comment has been minimized.

Copy link
Member

mszostok commented Nov 5, 2019

@The-Loeki I'm merging this pr because we will release new version today and is nice to have that fix. Thanks for contributing. If you will encounter any other problems then please submit another pr:)

Cheers!

/lgtm
/approve

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Nov 5, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mszostok, The-Loeki

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 merged commit 433a876 into kubernetes-sigs:master Nov 5, 2019
14 checks passed
14 checks passed
cla/linuxfoundation The-Loeki authorized
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details
pull-build-all-images-for-amd64 Job succeeded.
Details
pull-build-all-images-for-arm Job succeeded.
Details
pull-build-all-images-for-arm64 Job succeeded.
Details
pull-build-all-images-for-ppc64le Job succeeded.
Details
pull-build-all-images-for-s390x Job succeeded.
Details
pull-build-all-images-for-svcat Job succeeded.
Details
pull-service-catalog-test-e2e Job succeeded.
Details
pull-service-catalog-test-integration Job succeeded.
Details
pull-service-catalog-test-migration Job succeeded.
Details
pull-service-catalog-test-unit Job succeeded.
Details
tide In merge pool.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.