Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

1314902: ENT-555 use dmi.system.uuid to reconcile hypervisors #2035

Merged
merged 1 commit into from Jun 26, 2018

Conversation

vritant
Copy link
Contributor

@vritant vritant commented Jun 15, 2018

No description provided.

@@ -699,6 +700,71 @@ public VirtConsumerMap getHostConsumersMap(Owner owner, Iterable<String> hypervi
return hypervisorMap;
}

/**
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are two getHostConsumersMap methods (see method above). One is called by the job and the other in the resource. Each one is building the map in different ways. Is this intended? Why would sync vs async be different?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mstead async version has the fact we are using here, sync version does not.
only a recent version of virt-who would provide the fact in the report, and all recent versions should talk to the async end point anyway.

}
}

for (Consumer consumer: this.getConsumers(consumerIds)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe that this loop can be moved inside and at the end of the !systemUuidHypervisorMap.isEmpty() block since it is only applicable in this case (consumerIds is only used there).

@vritant
Copy link
Contributor Author

vritant commented Jun 19, 2018

@mstead done

@wottop wottop self-assigned this Jun 19, 2018
}
remainingHypervisorIds.add(consumer.getHypervisorId().getHypervisorId());
}
List<String> consumerIds = new LinkedList<>();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be moved into the block it is used in.

@wottop
Copy link
Member

wottop commented Jun 19, 2018

Code looks good, but.

If a consumer with the system.uuid comes in first, the hostname in CP is set to the system.uuid.
What happens when the hypervisor report comes in with a 'machine name' hostname?
I don't think you changed the behavior from what I had, but we should reconcile it. The Satellite folks will expect the hostname in the hypervisor report as soon as it is reported.

@vritant
Copy link
Contributor Author

vritant commented Jun 20, 2018

I made a mistake, the method I added slipped into a previous PR, but the method was not used there yet.

@wottop will chat with you online today.

@vritant
Copy link
Contributor Author

vritant commented Jun 25, 2018

@wottop added the logic, and a new unit test to test it too

@mstead
Copy link
Member

mstead commented Jun 26, 2018

Ooops.. Clicked the close button by mistake.

@vritant
Copy link
Contributor Author

vritant commented Jun 26, 2018

@wottop @mstead any objection to merge this PR?

@wottop
Copy link
Member

wottop commented Jun 26, 2018

ack

@wottop wottop merged commit 865364c into master Jun 26, 2018
@wottop wottop deleted the vritant/1314902 branch June 26, 2018 16:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants