Make periodic sync nodes from -cloud_provider/-machines optional. #3498
Conversation
|
@smarterclayton @ddysher please review |
|
This LGTM, but can you please file a CLA so we can accept it? (CONTRIBUTING.md) |
| @@ -48,6 +48,7 @@ var ( | |||
| cloudProvider = flag.String("cloud_provider", "", "The provider for cloud services. Empty string for no provider.") | |||
| cloudConfigFile = flag.String("cloud_config", "", "The path to the cloud provider configuration file. Empty string for no configuration file.") | |||
| minionRegexp = flag.String("minion_regexp", "", "If non empty, and -cloud_provider is specified, a regular expression for matching minion VMs.") | |||
| syncNodes = flag.Bool("sync_nodes", true, "If true, and -cloud_provider/-machines is specified, sync nodes from the cloud provider/machine list. Default true.") | |||
There was a problem hiding this comment.
Can you rename it to syncNodeList? Soonish, node controller will also sync node status, kill pod on down minions, etc, that part is not optional afaict.
|
Ravi is a Red Hat employee and covered under our CLA
|
pravisankar
force-pushed
the
ravi/sync-nodes-optional
branch
from
066f309 to
cec9b2b
Compare
|
@ddysher updated, please take a look |
pravisankar
force-pushed
the
ravi/sync-nodes-optional
branch
from
cec9b2b to
4d4e37b
Compare
pravisankar
force-pushed
the
ravi/sync-nodes-optional
branch
2 times, most recently
from
ba80340 to
fbe387d
Compare
|
travis is failing, can you double check code is go formatted? |
pravisankar
force-pushed
the
ravi/sync-nodes-optional
branch
2 times, most recently
from
71bc916 to
5fff370
Compare
pravisankar
force-pushed
the
ravi/sync-nodes-optional
branch
from
5fff370 to
a2d4ce0
Compare
|
need a rebase |
pravisankar
force-pushed
the
ravi/sync-nodes-optional
branch
from
a2d4ce0 to
0ee6e50
Compare
|
@ddysher rebase done |
|
sorry, looks like you need to rebase again.. Also, ur change is conflict with #3733. You can either wait until that one is in, or rebase earlier so this can go in quickly :) |
pravisankar
force-pushed
the
ravi/sync-nodes-optional
branch
2 times, most recently
from
b2f6164 to
4cc48fd
Compare
pravisankar
force-pushed
the
ravi/sync-nodes-optional
branch
from
4cc48fd to
cc73e74
Compare
bgrant0607
added
the
lgtm
|
Should be merged in the morning. |
|
@pravisankar k8s is under heavy development, I've rebased PRs several times before. With node status syncs, this is no longer the correct way to do this. We must keep node controller running for syncing node status You need to pass the param as a configuration to controller. |
|
@pravisankar There are some more changes going on with controller manager, can we have a fix? |
--sync_nodes=false gives user flexibility to add/remove nodes in the cluster using REST api/kubectl cli and at the same time can use cloud provider for other resources like persistent disks, etc.
pravisankar
force-pushed
the
ravi/sync-nodes-optional
branch
from
cc73e74 to
3a5ef05
Compare
|
LGTM |
Make periodic sync nodes from -cloud_provider/-machines optional.
-sync_nodes=false gives user flexibility to add/remove nodes in the
cluster using REST api/kubectl cli and at the same time can use
cloud provider for other resources like persistent disks, etc.