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

Per-node config #684

Merged
merged 17 commits into from
Jun 15, 2018
Merged

Per-node config #684

merged 17 commits into from
Jun 15, 2018

Conversation

ivan4th
Copy link
Contributor

@ivan4th ivan4th commented Jun 5, 2018

  • use k8s.io/code-generator for CRDs
  • implement configuration object
  • implement per-node config
  • testing (incl. virtletctl version which may break)
  • update image translation code to use generated CRD interfaces
  • CRD validation
  • style fixes
  • update docs

For now, the CRD-related generated code is updated using the following command, which only
works if Go 1.10 is installed locally:

build/update-codegen.sh

This is to be fixed shortly after we switch to Go 1.10 which is required by k8s code generator, so it'll be done via build/cmd.sh update-codegen (and the docs will be updated accordingly).


This change is Reviewable

Also, use k8s.io/code-generator for CRDs.
@ivan4th ivan4th force-pushed the ivan4th/per-node-config branch 2 times, most recently from 59ec825 to b27bbe8 Compare June 7, 2018 02:12
Ivan Shvedunov added 4 commits June 7, 2018 21:43
Standard output includes the CRDs
`virtletctl gen --crd` gives CRD-only output
Add perms for accessing nodes and CRDs
@ivan4th ivan4th changed the title [WiP] Per-node config Per-node config Jun 12, 2018
@jellonek
Copy link
Contributor

There are only a minor nits in docs, which imo can be fixed later so :lgtm_strong:


Reviewed 123 of 123 files at r1.
Review status: 1 of 2 LGTMs obtained


build/update-codegen.sh, line 11 at r1 (raw file):

CODEGEN_PKG="${CODEGEN_PKG:-$(cd ${SCRIPT_ROOT}; ls -d -1 ./vendor/k8s.io/code-generator 2>/dev/null || echo ${GOPATH}/src/k8s.io/code-generator)}"

vendor/k8s.io/code-generator/generate-groups.sh all \

this assumes that script is executed in root dir of working copy - it would be nice to add above this line cd ${SCRIPT_ROOT}


docs/config.md, line 46 at r1 (raw file):

You can delete Virtlet pods to have them restarted and pick up the

imo You need to delete Virtlet pods ....


docs/config.md, line 51 at r1 (raw file):

All the config mappings must reside in `kube-system` namespace.  Each
mapping can specify `nodeSelector` or `nodeName` to target a subset of
the nodes. If neither `nodeSelector` nor `nodeName` is specified, the

If you want to be consistent with double spaces after dots - there is one missing before If, same with nex if and later before config.


docs/devel/build-tool.md, line 122 at r1 (raw file):

### update-docs

Updates the documentation on virtletctl an the Virtlet config.

Please enclose virtletctl with `.


Comments from Reviewable

@pigmej
Copy link
Contributor

pigmej commented Jun 13, 2018

Reviewed 25 of 123 files at r1.
Review status: 1 of 2 LGTMs obtained


Comments from Reviewable

@lukaszo
Copy link
Contributor

lukaszo commented Jun 15, 2018

Review status: 1 of 2 LGTMs obtained


docs/config.md, line 9 at r1 (raw file):

In order to use per-node configuration, you need to create Virtlet
CRD definitions before you deploy Virtlet:

Restart of virtlet after changes should also work?


Comments from Reviewable

@jellonek
Copy link
Contributor

Review status: 1 of 2 LGTMs obtained


docs/config.md, line 9 at r1 (raw file):

Previously, lukaszo (Łukasz Oleś) wrote…

Restart of virtlet after changes should also work?

Yes, recreation of pod will cause usage of new values.


Comments from Reviewable

@lukaszo
Copy link
Contributor

lukaszo commented Jun 15, 2018

:lgtm:


Reviewed 123 of 123 files at r1.
Review status: 1 of 2 LGTMs obtained, and 1 stale


Comments from Reviewable

@jellonek jellonek merged commit 31a8672 into master Jun 15, 2018
@jellonek jellonek deleted the ivan4th/per-node-config branch June 15, 2018 10:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants