unify container and container definition #42

zeari · 2017-06-19T12:27:48Z

Depends on ManageIQ/manageiq#15393

@enoodle @zakiva Please review
cc @simon3z @moolitayer @cben

simon3z · 2017-06-22T12:15:16Z

@moolitayer @cben can you review?
cc @agrare @Ladas

cben

Small comments, overall LGTM

cben · 2017-06-25T12:31:00Z

app/models/manageiq/providers/kubernetes/container_manager/refresh_parser.rb

@@ -333,18 +332,16 @@ def parse_pod(pod)
      # TODO, podIP
      containers_index = {}
      containers = pod.spec.containers
+      containers.collect do |container_def|
+        containers_index[container_def.name] = parse_container_definition(container_def, pod.metadata.uid)


consider renaming
s/parse_container_definition(/parse_container_spec(/
s/parse_container(/parse_container_status(/
👍 on keeping the 2 functions separate, but since they now go to same table, the distinction that makes sense is the names of the input parts.

cben · 2017-06-25T12:32:54Z

app/models/manageiq/providers/kubernetes/container_manager/refresh_parser.rb

@@ -333,18 +332,16 @@ def parse_pod(pod)
      # TODO, podIP
      containers_index = {}
      containers = pod.spec.containers
+      containers.collect do |container_def|


.collect builds an array (aka .map), if you don't need it use .each.

cben · 2017-06-25T12:38:30Z

app/models/manageiq/providers/kubernetes/container_manager/refresh_parser.rb

      unless pod.status.nil? || pod.status.containerStatuses.nil?
        pod.status.containerStatuses.each do |cn|
-          containers_index[cn.name] = parse_container(cn, pod.metadata.uid)
+          containers_index[cn.name].merge!(parse_container(cn, pod.metadata.uid))


As dicussed, we can't think of a scenario containers_index[cn.name] could be nil (status without spec) but let's handle it just in case.

Another question: can a container that previously had a status stop having it?
I suspect in such case we'd not clear the fields last parsed from a status, I think save_inventory_multi only updates the fields you set in the hash (?)

i can set those as nil, but the question is if we would want to 'delete' the last known status

good question, hard to say without a scenario when this could happen (if at all)...
I think a good rule is that refresh should be "stateless", should converge to final state == what API gives, irrespective of what we saw before.
the exception to this rule is archiving. if you see a benefit to remembering last status seen, then archiving Container separately from ContainerDefinition would be a clean way to do this — a way that disappears after this change merging them.
IMO it's not worth the bother for a hypothetical situation, I'd set them to nil.

BTW, what do we do now? Would a ContainerDefinition's Container get deleted if [hypotetically] status disappears?

cben · 2017-06-25T14:02:55Z

Graph saving would also need updating, I'll help with that if you'll want to do it in this PR, ~~or I could do it in later PR. (graph refresh specs are currently broken anyway, marked pending)~~

zeari · 2017-07-02T13:39:51Z

Graph saving would also need updating, I'll help with that if you'll want to do it in this PR, or I could do it in later PR. (graph refresh specs are currently broken anyway, marked pending)

gladly, the less PRs the better.

miq-bot · 2017-07-03T13:34:06Z

This pull request is not mergeable. Please rebase and repush.

Ladas · 2017-07-03T15:56:39Z

👍 for simplifying of the model :-)

simon3z · 2017-07-08T12:23:20Z

@zeari if you rebase this I think we're ready to move forward with all this series (schema, ui, core and kubernetes provider).

cben · 2017-07-19T11:24:53Z

@zeari do tests pass locally given the dependencies?
I suppose the graph refresh test doesn't, you need to merge get_container_definitions_graph into get_containers_graph, I'm happy to help.

miq-bot · 2017-07-20T12:55:03Z

This pull request is not mergeable. Please rebase and repush.

zeari · 2017-07-20T13:23:01Z

@zeari do tests pass locally given the dependencies?
I suppose the graph refresh test doesn't, you need to merge get_container_definitions_graph into get_containers_graph, I'm happy to help.

@cben should be good now.

cben · 2017-07-20T13:28:41Z

app/models/manageiq/providers/kubernetes/container_manager/refresh_parser.rb

@@ -999,7 +991,7 @@ def parse_conditions(entity)
      end
    end

-    def parse_container_definition(container_def, pod_id)
+    def parse_container_spec(container_def, pod_id)


oh, can you rename container_def -> container_spec arg

zeari · 2017-07-20T14:11:47Z

oh, can you rename container_def -> container_spec arg

Roger Roger

miq-bot · 2017-07-21T08:39:18Z

This pull request is not mergeable. Please rebase and repush.

miq-bot · 2017-07-24T08:54:30Z

Checked commit zeari@b1bacea with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
3 files checked, 1 offense detected

app/models/manageiq/providers/kubernetes/container_manager/refresh_parser.rb

⚠️ - Line 1042, Col 43 - Lint/UnusedMethodArgument - Unused method argument - pod_id. If it's necessary, use _ or _pod_id as an argument name to indicate that it won't be used.

cben · 2017-07-26T15:00:39Z

@moolitayer Migrations & core have landed, we need this.
@zeari (or anyone) can you kick Travis?

cben · 2017-07-26T15:18:58Z

Still red but now less red.
@moolitayer I propose we merge this because PG::UndefinedTable: ERROR: relation \"container_definitions\" does not exist is really bad, while with this PR I'm able to run refresh...

I'm looking into the VCR unused HTTP interactions, will try to send PR.

The `.find_by` instead of `.first` will make the tested targets stable and matching the VCR even if refresh record order changes. It also happens to fix VCR errors following ManageIQ#42, but not because of order. Without .reload after refresh, ActiveRecord caches were stale, and pod.containers returned same container twice.

Fryguy · 2017-07-26T19:55:44Z

Rekicking this PR after merging #76

cben · 2017-07-26T20:23:32Z

Green 🎉

cben reviewed Jun 25, 2017

View reviewed changes

zeari force-pushed the unify_container_definition branch from 0e1fc3e to ae1a2bf Compare July 3, 2017 09:45

simon3z mentioned this pull request Jul 3, 2017

migrations for merging container_definition and container ManageIQ/manageiq-schema#24

Merged

miq-bot added the unmergeable label Jul 3, 2017

zeari force-pushed the unify_container_definition branch 4 times, most recently from 1ce53ea to 59c1ed4 Compare July 10, 2017 13:59

miq-bot removed the unmergeable label Jul 11, 2017

simon3z closed this Jul 11, 2017

simon3z reopened this Jul 11, 2017

zeari force-pushed the unify_container_definition branch from 59c1ed4 to 87e5405 Compare July 20, 2017 12:49

miq-bot added the unmergeable label Jul 20, 2017

zeari force-pushed the unify_container_definition branch from 87e5405 to 6046160 Compare July 20, 2017 13:04

miq-bot removed the unmergeable label Jul 20, 2017

zeari force-pushed the unify_container_definition branch from 6046160 to 4206496 Compare July 20, 2017 13:20

cben approved these changes Jul 20, 2017

View reviewed changes

cben reviewed Jul 20, 2017

View reviewed changes

zeari force-pushed the unify_container_definition branch from 4206496 to 726ace3 Compare July 20, 2017 14:11

miq-bot added the unmergeable label Jul 21, 2017

unify container and container definition

b1bacea

zeari force-pushed the unify_container_definition branch from 726ace3 to b1bacea Compare July 24, 2017 08:54

miq-bot removed the unmergeable label Jul 24, 2017

zeari closed this Jul 26, 2017

zeari reopened this Jul 26, 2017

cben mentioned this pull request Jul 26, 2017

Fix "unused HTTP interactions", make test more robust #76

Merged

Fryguy closed this Jul 26, 2017

Fryguy reopened this Jul 26, 2017

Fryguy merged commit 00df730 into ManageIQ:master Jul 26, 2017

Fryguy added this to the Sprint 66 Ending Aug 7, 2017 milestone Jul 26, 2017

Fryguy added the refactoring label Jul 26, 2017

simon3z added the inventory label Sep 5, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

unify container and container definition #42

unify container and container definition #42

zeari commented Jun 19, 2017

simon3z commented Jun 22, 2017

cben left a comment

cben Jun 25, 2017

cben Jun 25, 2017

cben Jun 25, 2017

cben Jun 25, 2017

zeari Jul 3, 2017

cben Jul 3, 2017

cben commented Jun 25, 2017 •

edited

Loading

zeari commented Jul 2, 2017

miq-bot commented Jul 3, 2017

Ladas commented Jul 3, 2017 •

edited

Loading

simon3z commented Jul 8, 2017

cben commented Jul 19, 2017

miq-bot commented Jul 20, 2017

zeari commented Jul 20, 2017

cben Jul 20, 2017

zeari commented Jul 20, 2017

miq-bot commented Jul 21, 2017

miq-bot commented Jul 24, 2017

cben commented Jul 26, 2017

cben commented Jul 26, 2017

Fryguy commented Jul 26, 2017

cben commented Jul 26, 2017

unify container and container definition #42

unify container and container definition #42

Conversation

zeari commented Jun 19, 2017

simon3z commented Jun 22, 2017

cben left a comment

Choose a reason for hiding this comment

cben Jun 25, 2017

Choose a reason for hiding this comment

cben Jun 25, 2017

Choose a reason for hiding this comment

cben Jun 25, 2017

Choose a reason for hiding this comment

cben Jun 25, 2017

Choose a reason for hiding this comment

zeari Jul 3, 2017

Choose a reason for hiding this comment

cben Jul 3, 2017

Choose a reason for hiding this comment

cben commented Jun 25, 2017 • edited Loading

zeari commented Jul 2, 2017

miq-bot commented Jul 3, 2017

Ladas commented Jul 3, 2017 • edited Loading

simon3z commented Jul 8, 2017

cben commented Jul 19, 2017

miq-bot commented Jul 20, 2017

zeari commented Jul 20, 2017

cben Jul 20, 2017

Choose a reason for hiding this comment

zeari commented Jul 20, 2017

miq-bot commented Jul 21, 2017

miq-bot commented Jul 24, 2017

cben commented Jul 26, 2017

cben commented Jul 26, 2017

Fryguy commented Jul 26, 2017

cben commented Jul 26, 2017

cben commented Jun 25, 2017 •

edited

Loading

Ladas commented Jul 3, 2017 •

edited

Loading