Issue #207 Stnd control plane node issue #302

renmak · 2017-03-23T04:24:41Z

What is the purpose of this pull request?: Updating Nova label name and value to align with how labels are used in Neutron

What issue does this pull request address?: Fixes issue #207. Also this new PR replaces previous PR #285

Notes for reviewers to consider: Please look at comment from Allan in PR #285. My new changes in this PR are based on his comment.

Specific reviewers for pull request: @alanmeadows @v1k0d3n

gardlt · 2017-03-23T18:30:19Z

should we try and propagate this changes to the other charts as well not just the keystone and compute ones. @intlabs @v1k0d3n @renmak

wilkers-steve · 2017-03-24T13:12:30Z

@renmak Thanks for taking this on. I looked through the comments on PR #285, and this gets us closer. However, I think it doesn't quite get us to where @alanmeadows suggested for the labels for nova. Instead of having a blanket approach to labels under a compute and control tree, I think the goal is to split this out per service that Nova runs. So instead of compute: or control:, it'd be conductor:, compute:, scheduler:, and so on -- at least that's my understand. Could you share your thoughts on that @alanmeadows?

renmak · 2017-03-27T19:52:13Z

@wilkers-steve @v1k0d3n @alanmeadows
So if we go by services, it would look like following

consoleauth: openstack-compute-node
scheduler: openstack-control-plane
conductor: openstack-control-plane
compute: openstack-control-plane
network:
api:

We will refer to above key value as
{{ .Values.labels**.scheduler.**node_selector_key }}

Is this what you guys have in mind? Thanks

alanmeadows · 2017-03-28T16:36:53Z

This looks good, except I agree with steve, lets just break it out, and simultaneously, achieve parity with neutron and its agent approach:

labels:
  agent:
    compute:
      node_sele...
  osapi: 
    node_sele...
  conductor:
    node_sele...

also, even though its outside this pull request, can we fix replicas as its now a bit strange with the removal of control and is a simple fix. See any other chart for how replicas has shaped up (except in nova)

replicas:
  api: ..
  conductor: ..
...and so on...

renmak · 2017-03-28T19:28:36Z

@alanmeadows @wilkers-steve @v1k0d3n @intlabs

Thanks Alan for feedback. So based on your feedback, this is what i came up with. Please correct me if this is incorrect.

So based on openstack-helm/nova/templates/ each, daemonset and deployment have been addressed below.

labels:
  agent:
    compute: 
      node_selector_key: openstack-compute-plane
      node_selector_value: enabled
    libvert:
      node_selector_key: openstack-control-plane
      node_selector_value: enabled
  conductor:
    node_selector_key: openstack-control-plane
    node_selector_value: enabled
  consoleauth:
    node_selector_key: openstack-control-plane
    node_selector_value: enabled
  scheduler:
    node_selector_key: openstack-control-plane
    node_selector_value: enabled
  osapi:
    node_selector_key: openstack-control-plane
    node_selector_value: enabled
  api-metadata:
    node_selector_key: openstack-control-plane
    node_selector_value: enabled
  server:
    node_selector_key: openstack-control-plane
    node_selector_value: enabled

alanmeadows · 2017-03-28T20:11:07Z

Libvirt isn't really an agent, but this could work for now

labels:
  agent:
    compute: 
      node_selector_key: openstack-compute-node
      node_selector_value: enabled
    libvirt:
      node_selector_key: openstack-compute-node
      node_selector_value: enabled

Note the openstack-compute-node for agent processes (those that run on compute hosts vs control plane)

All the other definitions are fine, but what is server: here for nova?

renmak · 2017-03-28T20:23:26Z

k that's my misunderstanding. server: should not be there.

But that brings up a question. We have total 5 jobs. db-init, db-sync, ks-endpoints, ks-service and ks-user. Each of these jobs refers to openstack-control-plane.

Which label do we use for these jobs?

alanmeadows · 2017-03-28T21:43:13Z

I would just add the below and have them refer to this. I do not think we have standardized this approach (allowing jobs (as a holistic category) to target their label), but a separate pull request could introduce this pattern everywhere:

labels:
  job:
    node_selector_key: openstack-control-plane
    node_selector_value: enabled

Defining a label for each job may be overkill.

renmak · 2017-03-28T22:10:01Z

Thanks for clarification Alan.

As per your suggestions on Replicas, here is what i was planning.

replicas:
  api: 1    #for both api-osapi & api-metadata. Should i also 
  conductor: 1
  consoleauth: 1
  scheduler: 1

I will make needed code changes and update this PR.

alanmeadows · 2017-03-29T02:59:36Z

All sounds fine except I would ensure metadata has its own replica declaration, much like api-metadata does. This gives you parity with labels.

Also, if we're calling it "osapi" consistently, lets use that for replicas as well.

The goal is consistent naming between the names within the deployment manifests themselves, the replica fields, and the label fields so that all three match as closely as possible.

wilkers-steve

@renmak This is looking good. A few things to address then this looks close to good for me

wilkers-steve · 2017-03-29T17:35:34Z

nova/templates/daemonset-libvirt.yaml

@@ -31,7 +31,7 @@ spec:
        ]'
    spec:
      nodeSelector:
-        {{ .Values.labels.compute_node_selector_key }}: {{ .Values.labels.compute_node_selector_value }}
+        {{ .Values.labels.agent.libvert.node_selector_key }}: {{ .Values.labels.agent.libvert.node_selector_value }}


s/libvert/libvirt

wilkers-steve · 2017-03-29T17:38:04Z

nova/values.yaml

+    compute:
+      node_selector_key: openstack-compute-plane
+      node_selector_value: enabled
+    libvert:


s/libvert/libvirt

wilkers-steve · 2017-03-29T17:43:15Z

nova/values.yaml

-compute_replicas: 1
+  agent:
+    compute:
+      node_selector_key: openstack-compute-plane


Should be openstack-compute-node instead of openstack-compute-plane

wilkers-steve · 2017-03-29T17:43:41Z

nova/values.yaml

+      node_selector_key: openstack-compute-plane
+      node_selector_value: enabled
+    libvert:
+      node_selector_key: openstack-compute-plane  


Also openstack-compute-node, not openstack-compute-plane

renmak · 2017-03-29T18:38:16Z

I will squash all these commits into one final commit

v1k0d3n · 2017-04-06T13:09:31Z

nova/templates/job-db-sync.yaml

@@ -28,7 +28,7 @@ spec:
    spec:
      restartPolicy: OnFailure
      nodeSelector:
-        {{ .Values.labels.control_node_selector_key }}: {{ .Values.labels.control_node_selector_value }}
+        {{ .Values.labels.job.node_selector_key }}: {{ .Values.labels.job.node_selector_value }}


are we going to assign jobs to different nodes than our deployments?

Though unlikely there is a small possibility that some people may want to do this; as these pods will contain things like root db creds that the wider control-plane population will never see.

fair enough. sounds good to me. :)

v1k0d3n

LGTM. @wilkers-steve or other cores, i think we can merge this in when you're ready.

larryrensing · 2017-04-06T18:48:26Z

nova/templates/deployment-api-metadata.yaml

@@ -19,7 +19,7 @@ kind: Deployment
 metadata:
  name: nova-api-metadata
 spec:
-  replicas: {{ .Values.control_replicas }}
+  replicas: {{ .Values.replicas.api-metadata }}


Helm won't be able to package up the chart with .Values.replicas.api-metadata. You can only use alphanumeric and underscores in YAML keys. Something like .Values.replicas.api_metadata will build.

Good catch @larryrensing

larryrensing · 2017-04-06T18:48:35Z

nova/templates/deployment-api-metadata.yaml

@@ -40,7 +40,7 @@ spec:
        ]'
    spec:
      nodeSelector:
-        {{ .Values.labels.control_node_selector_key }}: {{ .Values.labels.control_node_selector_value }}
+        {{ .Values.labels.api-metadata.node_selector_key }}: {{ .Values.labels.api-metadata.node_selector_value }}


larryrensing · 2017-04-06T18:49:16Z

nova/values.yaml

+    node_selector_value: enabled
+
+replicas:
+  api-metadata: 1


larryrensing · 2017-04-06T18:49:23Z

nova/values.yaml

+  osapi:
+    node_selector_key: openstack-control-plane
+    node_selector_value: enabled
+  api-metadata:


larryrensing · 2017-04-06T18:49:49Z

nova/values.yaml

@@ -16,15 +16,39 @@
 # This is a YAML-formatted file.
 # Declare name/value pairs to be passed into your templates.
 # name: value
-


keep this space

wilkers-steve

@renmak, @larryrensing pointed out the remaining issues that should be addressed before we merge this in. Once those are complete, I think this should be good.

renmak · 2017-04-06T22:28:44Z

Thanks for review and catching bug. I will work on this today and get code changes in.

v1k0d3n

nice work @renmak, and good catch @larryrensing. LGTM.

…nd Replicas for consistency in values.yaml.

renmak · 2017-04-06T22:53:25Z

K so update one final commit with all changes. Thanks @v1k0d3n @wilkers-steve @larryrensing
for reviews.

wilkers-steve · 2017-04-07T13:13:53Z

@renmak This looks good. Thanks for fixing this. Pulled it down locally and everything runs fine. I'll go ahead and merge this in once @larryrensing signs off on the changes being satisfactory

intlabs · 2017-04-07T15:16:03Z

LGTM :) I'd merge @wilkers-steve

v1k0d3n added the in progress label Mar 23, 2017

mcnanci assigned renmak Mar 24, 2017

v1k0d3n added this to the 0.3.0 milestone Mar 29, 2017

wilkers-steve suggested changes Mar 29, 2017

View reviewed changes

renmak force-pushed the stnd_control_plane_node_issue_207 branch 2 times, most recently from 329d281 to fd953d8 Compare March 29, 2017 18:58

renmak mentioned this pull request Mar 31, 2017

Address Naming Convention for Entire Project #267

Open

wilkers-steve approved these changes Apr 6, 2017

View reviewed changes

v1k0d3n reviewed Apr 6, 2017

View reviewed changes

v1k0d3n self-requested a review April 6, 2017 14:07

v1k0d3n approved these changes Apr 6, 2017

View reviewed changes

larryrensing suggested changes Apr 6, 2017

View reviewed changes

wilkers-steve suggested changes Apr 6, 2017

View reviewed changes

v1k0d3n self-requested a review April 6, 2017 22:47

v1k0d3n approved these changes Apr 6, 2017

View reviewed changes

Fixed issue att-comdev#207 by Updated Nova’s Label (key/value pair) a…

f020ef6

…nd Replicas for consistency in values.yaml.

renmak force-pushed the stnd_control_plane_node_issue_207 branch from 5f90fd1 to f020ef6 Compare April 6, 2017 22:50

wilkers-steve approved these changes Apr 7, 2017

View reviewed changes

larryrensing approved these changes Apr 7, 2017

View reviewed changes

wilkers-steve merged commit 4cec28b into att-comdev:master Apr 7, 2017

v1k0d3n removed the in progress label Apr 7, 2017

wilkers-steve mentioned this pull request Apr 7, 2017

Updated label for nova-bootstrap job to match new labels for nova #333

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue #207 Stnd control plane node issue #302

Issue #207 Stnd control plane node issue #302

renmak commented Mar 23, 2017

gardlt commented Mar 23, 2017 •

edited

Loading

wilkers-steve commented Mar 24, 2017

renmak commented Mar 27, 2017

alanmeadows commented Mar 28, 2017

renmak commented Mar 28, 2017 •

edited

Loading

alanmeadows commented Mar 28, 2017 •

edited

Loading

renmak commented Mar 28, 2017

alanmeadows commented Mar 28, 2017 •

edited

Loading

renmak commented Mar 28, 2017

alanmeadows commented Mar 29, 2017

wilkers-steve left a comment

wilkers-steve Mar 29, 2017

wilkers-steve Mar 29, 2017

wilkers-steve Mar 29, 2017

wilkers-steve Mar 29, 2017

renmak commented Mar 29, 2017

v1k0d3n Apr 6, 2017

intlabs Apr 6, 2017

v1k0d3n Apr 6, 2017

v1k0d3n left a comment

larryrensing Apr 6, 2017

wilkers-steve Apr 6, 2017

larryrensing Apr 6, 2017

larryrensing Apr 6, 2017

larryrensing Apr 6, 2017

larryrensing Apr 6, 2017

wilkers-steve left a comment

renmak commented Apr 6, 2017

v1k0d3n left a comment •

edited

Loading

renmak commented Apr 6, 2017

wilkers-steve commented Apr 7, 2017

intlabs commented Apr 7, 2017

Issue #207 Stnd control plane node issue #302

Issue #207 Stnd control plane node issue #302

Conversation

renmak commented Mar 23, 2017

gardlt commented Mar 23, 2017 • edited Loading

wilkers-steve commented Mar 24, 2017

renmak commented Mar 27, 2017

alanmeadows commented Mar 28, 2017

renmak commented Mar 28, 2017 • edited Loading

alanmeadows commented Mar 28, 2017 • edited Loading

renmak commented Mar 28, 2017

alanmeadows commented Mar 28, 2017 • edited Loading

renmak commented Mar 28, 2017

alanmeadows commented Mar 29, 2017

wilkers-steve left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

renmak commented Mar 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

v1k0d3n left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wilkers-steve left a comment

Choose a reason for hiding this comment

renmak commented Apr 6, 2017

v1k0d3n left a comment • edited Loading

Choose a reason for hiding this comment

renmak commented Apr 6, 2017

wilkers-steve commented Apr 7, 2017

intlabs commented Apr 7, 2017

gardlt commented Mar 23, 2017 •

edited

Loading

renmak commented Mar 28, 2017 •

edited

Loading

alanmeadows commented Mar 28, 2017 •

edited

Loading

alanmeadows commented Mar 28, 2017 •

edited

Loading

v1k0d3n left a comment •

edited

Loading