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

Keep node labels in sync with Kubernetes #336

Merged
merged 10 commits into from Jan 4, 2019

Conversation

Projects
None yet
2 participants
@caseydavenport
Copy link
Member

caseydavenport commented Dec 17, 2018

Description

Builds on top of this PR, since @briansan is out this week.

#328

Todos

  • Tests
  • Documentation
  • Release note

Release Note

Calico nodes now inherit Kubernetes node labels

@caseydavenport caseydavenport referenced this pull request Dec 17, 2018

Closed

WIP: sync nodes labels from k8s node to calico node #328

0 of 3 tasks complete

@caseydavenport caseydavenport changed the title Briansan kdd node apply labels Keep node labels in sync with Kubernetes Dec 17, 2018

@caseydavenport caseydavenport force-pushed the caseydavenport:briansan-kdd-node-apply-labels branch 6 times, most recently from dcb2aff to 3dda468 Dec 17, 2018

@caseydavenport caseydavenport force-pushed the caseydavenport:briansan-kdd-node-apply-labels branch from 3dda468 to 6866ccf Dec 17, 2018

@caseydavenport caseydavenport added this to the Calico v3.5.0 milestone Dec 18, 2018

// On failure, we retry a certain number of times.
for n := 1; n < maxAttempts; n++ {
// Get the Calico node representation.
name, ok := nc.nodemapper[node.Name]

This comment was marked as resolved.

@caseydavenport

caseydavenport Jan 2, 2019

Author Member

I think we need to add a mutex on nodemapper access

continue
// If there are labels present, then parse them. Otherwise this is
// a first-time sync, in which case there are no old labels.
var oldLabels map[string]string = map[string]string{}

This comment has been minimized.

@briansan

briansan Jan 2, 2019

Member

why not just oldLabels := map[string]string{}

// If there are labels present, then parse them. Otherwise this is
// a first-time sync, in which case there are no old labels.
var oldLabels map[string]string = map[string]string{}
if ok {

This comment has been minimized.

@briansan

briansan Jan 2, 2019

Member

could we do a oneliner here e.g.

if a, ok := calNode.Annotations["..."]; ok {
  ...
}
// a first-time sync, in which case there are no old labels.
var oldLabels map[string]string = map[string]string{}
if ok {
json.Unmarshal([]byte(a), &oldLabels)

This comment has been minimized.

@briansan

briansan Jan 2, 2019

Member

should we error check here?

bapi "github.com/projectcalico/libcalico-go/lib/backend/api"
"github.com/projectcalico/libcalico-go/lib/backend/model"
"github.com/projectcalico/libcalico-go/lib/backend/watchersyncer"
"github.com/sirupsen/logrus"

This comment has been minimized.

@briansan

briansan Jan 2, 2019

Member

assign package to log for consistency?

This comment has been minimized.

@caseydavenport

caseydavenport Jan 3, 2019

Author Member

I think we're already inconsistent with how we do this, but I'm fine either way.

for _, upd := range updates {
switch upd.UpdateType {
case bapi.UpdateTypeKVNew:
fallthrough

This comment has been minimized.

@briansan
Eventually(func() *api.Node {
node, _ := c.Nodes().Get(context.Background(), cNodeName, options.GetOptions{})
return node
}, time.Second*2, 500*time.Millisecond).Should(BeNil())

This comment has been minimized.

@briansan

briansan Jan 3, 2019

Member

nice testing; straightforward and easy to understand

}

// Set the annotation to the correct values.
bytes, err := json.Marshal(node.Labels)

This comment has been minimized.

@briansan

briansan Jan 3, 2019

Member

i have a suspicion that it is impossible for json.Marshal to return an error here since node.Labels is a map[string]string and there no conceivable value we could set it to that would trigger an error.

would be okay to ignore the error in these kinds of cases?
reference: https://stackoverflow.com/questions/33903552/what-input-will-cause-golangs-json-marshal-to-return-an-error

This comment has been minimized.

@caseydavenport

caseydavenport Jan 3, 2019

Author Member

Yeah, it probably is. But, I think we have checks which will fail if we ignore errors.

I don't mind having it there anyway, just in case....

@briansan briansan referenced this pull request Jan 3, 2019

Merged

address pr comments #1

if cfg.SyncNodeLabels {
// Start the syncer.
nc.initSyncer()
nc.syncer.Start()

This comment has been minimized.

@briansan

briansan Jan 4, 2019

Member

I think we end up with a weird race condition where the syncer starts and tries to process updates for the Calico Node objects where it attempts to access the indexer which hasn't been initialized yet. Would switching around the order of initialization fix this?

This comment has been minimized.

@caseydavenport

caseydavenport Jan 4, 2019

Author Member

It's intended that it doesn't matter which one occurs first - it should succeed when the other sends an update.

Merge pull request #2 from briansan/briansan-kdd-node-apply-labels
setup calico syncer after kubernetes syncer

@caseydavenport caseydavenport merged commit 8eee94b into projectcalico:master Jan 4, 2019

2 checks passed

license/cla Contributor License Agreement is signed.
Details
semaphoreci The build passed on Semaphore.
Details

@caseydavenport caseydavenport deleted the caseydavenport:briansan-kdd-node-apply-labels branch Jan 4, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment