-
Notifications
You must be signed in to change notification settings - Fork 250
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
Fixing calico-ipam using hostname instead of nodename #375
Conversation
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.
This breaks upgrade, I think.
lgtm at this point. |
ipam/calico-ipam.go
Outdated
@@ -69,12 +69,26 @@ type ipamArgs struct { | |||
IP net.IP `json:"ip,omitempty"` | |||
} | |||
|
|||
func updateNodename(conf utils.NetConf) string { |
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.
naming nit: this isn't updating nodename, it's determining it.
Maybe determineNodename
would be better.
ipam/calico-ipam.go
Outdated
func cmdAdd(args *skel.CmdArgs) error { | ||
conf := utils.NetConf{} | ||
if err := json.Unmarshal(args.StdinData, &conf); err != nil { | ||
return fmt.Errorf("failed to load netconf: %v", err) | ||
} | ||
|
||
nodename := updateNodename(conf) | ||
|
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.
no need for the extra newline.
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.
A couple of minor nits.
Once those are fixed and then this is squashed feel free to merge.
It looks as though we were still using the deprecated `hostname` from the CNI payload, and not `nodename` which was causing issues assigning affine blocks if you need to use a `NODENAME` that is different from the Nodes `os.Hostname()`. We'll check for the old `hostname` value as well similar to how we do in the CNI plugin.
9653b24
to
12f2233
Compare
Description
Fixes projectcalico/calico#826
It looks as though we were still using the deprecated
hostname
from the CNI payload, and notnodename
which was causing issues assigning affine blocks if you need to use aNODENAME
that is different from the Nodesos.Hostname()
.I tested this on a vagrant cluster launching Calico with systemd to configure a
NODENAME
different than the Nodes hostname, and matched theNODENAME
env var in the CNI config filesnodename
. Without this fix the claimed blocks get assigned to the Nodes hostname in etcd, instead of the proper nodename.Todos
Release Note