Make sure Container has always the right STI type #177

Ladas · 2017-11-27T08:53:04Z

Make sure Container has always the right STI type. Before, the
missing image could cause the STI type is Container, which
made metrics capture fail.

Fixes BZ:
https://bugzilla.redhat.com/show_bug.cgi?id=1517676

Make sure Container has always the right STI type. Before, the missing image could cause the STI type is Container, which made metrics capture fail. Fixes BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1517676

miq-bot · 2017-11-27T08:59:12Z

Checked commit Ladas@561dba9 with ruby 2.3.3, rubocop 0.47.1, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 0 offenses detected
Everything looks fine. 🍪

Ladas · 2017-11-27T09:05:15Z

cc @simon3z

bazulay · 2017-11-27T10:41:02Z

@moolitayer PTAL

cben

great find 👍 🏅

cben · 2017-11-29T00:05:51Z

gaprindashvili/?

Ladas · 2017-11-29T06:59:28Z

@cben should be g-release, or even more?

cben · 2017-11-29T15:20:47Z

@miq-bot add-label gaprindashvili/yes
https://github.com/ManageIQ/manageiq-providers-kubernetes/blame/gaprindashvili/app/models/manageiq/providers/kubernetes/container_manager/refresh_parser.rb#L1082

I think fine/yes euwe/yes darga/yes capablanca/yes?! too (will need manual backport):
https://github.com/ManageIQ/manageiq/blame/fine/app/models/manageiq/providers/kubernetes/container_manager/refresh_parser.rb#L655
https://github.com/ManageIQ/manageiq/blame/euwe/app/models/manageiq/providers/kubernetes/container_manager/refresh_parser.rb#L628
https://github.com/ManageIQ/manageiq/blame/darga/app/models/manageiq/providers/kubernetes/container_manager/refresh_parser.rb#L631
https://github.com/ManageIQ/manageiq/blame/capablanca/app/models/manageiq/providers/kubernetes/container_manager/refresh_parser.rb#L522
type was set in parse_container (which became parse_container_status), not parse_container_definition (which became parse_container_spec).
@zeari did I read that correctly?
@bazulay how far back do we maintain?

cben · 2017-11-29T17:18:11Z

On second thoughts, I think on fine and older, before ContainerDefinition merge into Container, there is no bug.
Yes, container_status is not always present, but then Container record is just not created at all, and no metrics is collected, so no error message. IIUC only very new, not yet running, containers have no status, so not collecting metrics is fine.
@zeari please double-check my thinking.

And on gaprindashvili and master, the BZ symptom is not very severe, if it only happens to very new containers (though inventory may be stale, we might already get some metrics by time we collect).
But in any case having wrong STI type can lead to many weird problems, important to fix!

moolitayer · 2017-11-30T15:42:35Z

@Ladas thanks for the fix.
Could you please add a spec that fails before this fix?
(parsing is enough, refresh_parser_spec.rb)

zeari · 2017-11-30T16:34:54Z

On second thoughts, I think on fine and older, before ContainerDefinition merge into Container, there is no bug.
Yes, container_status is not always present, but then Container record is just not created at all, and no metrics is collected, so no error message. IIUC only very new, not yet running, containers have no status, so not collecting metrics is fine.
@zeari please double-check my thinking.

I agree, this is a new issue. either way if it does pop up we have @Ladas manual fix in https://bugzilla.redhat.com/show_bug.cgi?id=1517676

Ladas · 2017-12-04T07:59:25Z

@moolitayer @cben hm, could you guys do that magic with manual editing VCRs? In our VCR data, we do not have Container without image.

cben · 2017-12-04T09:32:49Z

I'd like to avoid manual editing, but I'm right now working on rerecording a VCR, I'll try to add such container (eg. a ~~bad image url should reliably result in container without status~~).

enoodle

Great Catch LGTM!

moolitayer · 2017-12-07T12:34:21Z

@Ladas refresh_parser_spec.rb does not use VCR and would be enough, thanks

Ladas · 2017-12-08T09:26:59Z

@cben not sure if refresh_parser_spec.rb is good for this? I see you use it mainly for 'it will be parsed good'. While in other providers, this is more of refresher_spec makes sure the right STI type is always there. (also in other providers, we are not setting the type in parser, unless there are sub STI types)

Ladas · 2017-12-11T14:55:02Z

@cben @moolitayer is it ok to just merge this to deal with the blocker BZ? Then @cben would cover this later with his VCR specs? Including this data combination example.

cben · 2017-12-11T15:08:04Z

Ok with me, FWIW

moolitayer · 2017-12-11T15:36:04Z

@Ladas why not add a refresh_parser_spec for the case that went wrong here? we use it as general unit tests related to all things parsing, STI and others. let's reiterate on that if needed

Ladas · 2017-12-11T16:02:12Z

@moolitayer looking at the refresh_parser_spec, you use that mainly for making sure the data are parsed correctly. For this case it's better to have VCR spec, testing the scenarios with missing and present image of a pod, leading to correct data. At least that is what we do in other providers. :-) Since @cben will be adding those VCR specs, I think we can leave it on that?

moolitayer · 2017-12-11T16:32:07Z

@Ladas It would be good to test in VCR specs as an integration test for the complete refresh and in refresh_parser_spec as a unit test - we already check several type attributes in that class.
Since that test is simple enough + fails before the PR and succeeds with it I would feel more confident having the test.

We used to have a requirement at some point to have both types of specs for each new entity.

…_right_sti_type Make sure Container has always the right STI type (cherry picked from commit cc36180) https://bugzilla.redhat.com/show_bug.cgi?id=1524630

simaishi · 2017-12-11T18:32:05Z

Gaprindashvili backport details:

$ git log -1
commit e319f3a7d92ab5f9f5ecc5355c442822a3a692ec
Author: Mooli Tayer <[email protected]>
Date:   Mon Dec 11 17:36:01 2017 +0200

    Merge pull request #177 from Ladas/make_sure_container_has_always_the_right_sti_type
    
    Make sure Container has always the right STI type
    (cherry picked from commit cc3618091bc5027b6ac0fee091555efe059cee86)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1524630

cben · 2018-01-25T13:41:04Z

@Ladas, does this need a data migration?
I just simulated via Container.last.update(type: "Container") and then refresh — it does NOT fix the type column. Neither :batch strategy graph nor classic refresh.
(if I destroy the Container and refresh it re-creates correctly of course)

P.S. found stable way to reproduce missing containerStatuses — a Pending pod — https://bugzilla.redhat.com/show_bug.cgi?id=1524630#c9

Ladas · 2018-01-25T13:46:39Z

@cben yeah. We do not update :type in refresh, for some reason. At this point, we should document a rails c fix, since it's too late for a migration.

Make sure Container has always the right STI type

561dba9

Make sure Container has always the right STI type. Before, the missing image could cause the STI type is Container, which made metrics capture fail. Fixes BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1517676

Ladas added the bug label Nov 27, 2017

cben approved these changes Nov 27, 2017

View reviewed changes

miq-bot added the gaprindashvili/yes label Nov 29, 2017

moolitayer added the inventory label Nov 30, 2017

Ladas mentioned this pull request Dec 4, 2017

Add container events to pod ManageIQ/manageiq#16583

Merged

moolitayer requested review from enoodle and zeari December 4, 2017 11:14

enoodle approved these changes Dec 4, 2017

View reviewed changes

zeari approved these changes Dec 4, 2017

View reviewed changes

moolitayer merged commit cc36180 into ManageIQ:master Dec 11, 2017

moolitayer added this to the Sprint 75 Ending Dec 11, 2017 milestone Dec 11, 2017

simaishi added gaprindashvili/backported and removed gaprindashvili/yes labels Dec 11, 2017

cben mentioned this pull request Dec 27, 2017

Tests for parse_pod, specifically for STI type if missing pod status #194

Merged

moolitayer self-assigned this Dec 27, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make sure Container has always the right STI type #177

Make sure Container has always the right STI type #177

Ladas commented Nov 27, 2017

miq-bot commented Nov 27, 2017

Ladas commented Nov 27, 2017

bazulay commented Nov 27, 2017

cben left a comment

cben commented Nov 29, 2017

Ladas commented Nov 29, 2017

cben commented Nov 29, 2017

cben commented Nov 29, 2017

moolitayer commented Nov 30, 2017

zeari commented Nov 30, 2017

Ladas commented Dec 4, 2017

cben commented Dec 4, 2017 via email •

edited

Loading

enoodle left a comment

moolitayer commented Dec 7, 2017

Ladas commented Dec 8, 2017

Ladas commented Dec 11, 2017

cben commented Dec 11, 2017 via email •

edited

Loading

moolitayer commented Dec 11, 2017

Ladas commented Dec 11, 2017

moolitayer commented Dec 11, 2017

simaishi commented Dec 11, 2017

cben commented Jan 25, 2018

Ladas commented Jan 25, 2018

Make sure Container has always the right STI type #177

Make sure Container has always the right STI type #177

Conversation

Ladas commented Nov 27, 2017

miq-bot commented Nov 27, 2017

Ladas commented Nov 27, 2017

bazulay commented Nov 27, 2017

cben left a comment

Choose a reason for hiding this comment

cben commented Nov 29, 2017

Ladas commented Nov 29, 2017

cben commented Nov 29, 2017

cben commented Nov 29, 2017

moolitayer commented Nov 30, 2017

zeari commented Nov 30, 2017

Ladas commented Dec 4, 2017

cben commented Dec 4, 2017 via email • edited Loading

enoodle left a comment

Choose a reason for hiding this comment

moolitayer commented Dec 7, 2017

Ladas commented Dec 8, 2017

Ladas commented Dec 11, 2017

cben commented Dec 11, 2017 via email • edited Loading

moolitayer commented Dec 11, 2017

Ladas commented Dec 11, 2017

moolitayer commented Dec 11, 2017

simaishi commented Dec 11, 2017

cben commented Jan 25, 2018

Ladas commented Jan 25, 2018

cben commented Dec 4, 2017 via email •

edited

Loading

cben commented Dec 11, 2017 via email •

edited

Loading