Don't leave trailing hyphen in deployment name #22419

jrafanie · 2023-03-17T19:43:12Z

If the ems_id is nil, empty, or not in the expected format, it can return nil
causing us to leave the deployment name with a trailing hyphen. Now, we only
append the hyphen and ems_id if it's present.
ems_id cannot be an array, simplifying this logic of ems_id returning nil or an fixnum

ems_id comes from ems_id_from_queue_name which as of [1] can only ever return an
integer or nil from parse_ems_id.

[1] #20345

Related, we need to fix the foreman provider to not return an array from queue_name_for_ems

agrare · 2023-03-20T12:49:18Z

spec/models/miq_worker/container_common_spec.rb

@@ -113,6 +113,35 @@ def deployment_name_for(name)
        expect(klass.constantize.new.worker_deployment_name.length).to be <= 60
      end
    end
+
+    context "ems_id" do
+      let(:subject) { ManageIQ::Providers::Openshift::ContainerManager::EventCatcher.new }


Does this have to be an Openshift::ContainerManager::EventCatcher for the specs? Would prefer not to call out a specific provider if possible. Could you use ManageIQ::Providers::BaseManager::EventCatcher ?

Fryguy · 2023-03-20T13:22:53Z

spec/models/miq_worker/container_common_spec.rb

+      end
+
+      it "isn't appended for array queue_name" do
+        subject.queue_name = %w[ems_1 ems2]


This is the only case I'm not understanding. If we have an "Array" queue_name, then the worker doesn't have an id? what happens if you have 2 separate queue_names like %w[ems_1 ems2] and %w[ems_3 ems4]? Wouldn't those collide?

I questioned that too. An array queue_name isn't supported as it was dropped in #20345.

I could just drop this test as it was just confirmation of how to fix the current problem. This deployment name creation code is so far from the source problem that raising in the code here would be quite unhelpful.

Ok, so that's telling me that foreman has a bug and needs to be fixed

Fryguy · 2023-03-20T13:23:26Z

spec/models/miq_worker/container_common_spec.rb

+
+      it "is appended" do
+        subject.queue_name = "ems_1"
+        expect(subject.worker_deployment_name[-1]).to eq("1")


This should include the hyphen so as not to allow any false positives like -21.

Suggested change

expect(subject.worker_deployment_name[-1]).to eq("1")

expect(subject.worker_deployment_name[-2..]).to eq("-1")

spec/models/miq_worker/container_common_spec.rb

Fryguy · 2023-03-20T13:26:25Z

Related, we need to fix the foreman provider to not return an array from queue_name_for_ems

Is this the only worker like this? If so, I think it's better to fix that, and then not have the special case.

agrare · 2023-03-20T13:51:33Z

Is this the only worker like this? If so, I think it's better to fix that, and then not have the special case.

We should not be returning arrays for ems_id anymore, foreman was just missed because it doesn't follow the typical parent/child manager pattern.

agrare · 2023-03-20T14:36:46Z

Opened ManageIQ/manageiq-providers-foreman#113 to fix the issue with foreman queue_name_for_ems returning an Array

jrafanie · 2023-03-21T16:01:56Z

Ok, I got back to this and pushed up the suggestions. Thanks.

miq-bot · 2023-03-21T16:07:26Z

Checked commits jrafanie/manageiq@047680d~...29dd755 with ruby 2.6.10, rubocop 1.28.2, haml-lint 0.35.0, and yamllint
2 files checked, 1 offense detected

app/models/miq_worker/container_common.rb

⚠️ - Line 111, Col 117 - Lint/UnifiedInteger - Use Integer instead of Fixnum.

Fryguy · 2023-03-28T15:25:01Z

app/models/miq_worker/container_common.rb

@@ -108,7 +108,7 @@ def deployment_prefix
    def worker_deployment_name
      @worker_deployment_name ||= begin
        deployment_name = abbreviated_class_name.dup.chomp("Worker").sub("Manager", "").sub(/^Miq/, "")
-        deployment_name << "-#{Array(ems_id).map { |id| ApplicationRecord.split_id(id).last }.join("-")}" if respond_to?(:ems_id)
+        deployment_name << "-#{ApplicationRecord.split_id(ems_id).last}" if respond_to?(:ems_id) && ems_id.kind_of?(Fixnum)


@jrafanie As discussed, let's change the kind_of?(FixNum) to a nil/presence check. Otherwise this LGTM.

ok, done. PTAL

If the ems_id is nil, empty, or not in the expected format, it can return nil, causing us to leave the deployment name with a trailing hyphen. Now, we only append the hyphen and ems_id if it's present.

ems_id comes from ems_id_from_queue_name which as of [1] can only ever return an integer or nil from parse_ems_id. [1] ManageIQ#20345

Fryguy · 2023-03-30T16:23:13Z

Backported to petrosian in commit 9b15782.

commit 9b15782a9c5af5443ab46eba5322447456b65807
Author: Jason Frey <[email protected]>
Date:   Wed Mar 29 15:18:29 2023 -0400

    Merge pull request #22419 from jrafanie/handle_invalid_ems_id_corrupting_deployment_name
    
    Don't leave trailing hyphen in deployment name
    
    (cherry picked from commit e845df9b5b7d4936d57488ed5a0ab906250b978f)

…ing_deployment_name Don't leave trailing hyphen in deployment name (cherry picked from commit e845df9)

jrafanie added bug build/podified labels Mar 17, 2023

jrafanie assigned agrare Mar 17, 2023

jrafanie requested a review from Fryguy as a code owner March 17, 2023 19:43

agrare reviewed Mar 20, 2023

View reviewed changes

Fryguy reviewed Mar 20, 2023

View reviewed changes

spec/models/miq_worker/container_common_spec.rb Outdated Show resolved Hide resolved

agrare mentioned this pull request Mar 20, 2023

Delegate Provisioning refresh to Configuration ManageIQ/manageiq-providers-foreman#113

Merged

jrafanie force-pushed the handle_invalid_ems_id_corrupting_deployment_name branch from f0715d8 to 29dd755 Compare March 21, 2023 16:00

Fryguy reviewed Mar 28, 2023

View reviewed changes

jrafanie added 2 commits March 29, 2023 11:40

Don't leave trailing hyphen in deployment name

abacbb0

If the ems_id is nil, empty, or not in the expected format, it can return nil, causing us to leave the deployment name with a trailing hyphen. Now, we only append the hyphen and ems_id if it's present.

ems_id cannot be an array, simplifying this logic

0686ffd

ems_id comes from ems_id_from_queue_name which as of [1] can only ever return an integer or nil from parse_ems_id. [1] ManageIQ#20345

jrafanie force-pushed the handle_invalid_ems_id_corrupting_deployment_name branch from 29dd755 to 0686ffd Compare March 29, 2023 15:41

agrare approved these changes Mar 29, 2023

View reviewed changes

Fryguy added the petrosian/yes label Mar 29, 2023

Fryguy approved these changes Mar 29, 2023

View reviewed changes

Fryguy merged commit e845df9 into ManageIQ:master Mar 29, 2023

Fryguy assigned Fryguy and unassigned agrare Mar 29, 2023

Fryguy added a commit that referenced this pull request Mar 30, 2023

Merge pull request #22419 from jrafanie/handle_invalid_ems_id_corrupt…

9b15782

…ing_deployment_name Don't leave trailing hyphen in deployment name (cherry picked from commit e845df9)

Fryguy added petrosian/backported and removed petrosian/yes labels Mar 30, 2023

jrafanie deleted the handle_invalid_ems_id_corrupting_deployment_name branch April 3, 2023 15:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't leave trailing hyphen in deployment name #22419

Don't leave trailing hyphen in deployment name #22419

jrafanie commented Mar 17, 2023 •

edited

Loading

agrare Mar 20, 2023

Fryguy Mar 20, 2023

jrafanie Mar 20, 2023

Fryguy Mar 20, 2023

Fryguy Mar 20, 2023

jrafanie Mar 20, 2023

Fryguy commented Mar 20, 2023

agrare commented Mar 20, 2023

agrare commented Mar 20, 2023

jrafanie commented Mar 21, 2023

miq-bot commented Mar 21, 2023

Fryguy Mar 28, 2023

jrafanie Mar 29, 2023

Fryguy commented Mar 30, 2023

	expect(subject.worker_deployment_name[-1]).to eq("1")
	expect(subject.worker_deployment_name[-2..]).to eq("-1")

Don't leave trailing hyphen in deployment name #22419

Don't leave trailing hyphen in deployment name #22419

Conversation

jrafanie commented Mar 17, 2023 • edited Loading

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

Fryguy commented Mar 20, 2023

agrare commented Mar 20, 2023

agrare commented Mar 20, 2023

jrafanie commented Mar 21, 2023

miq-bot commented Mar 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fryguy commented Mar 30, 2023

jrafanie commented Mar 17, 2023 •

edited

Loading