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

Move to using custom Ingestor for cloud resource specifications #1489

Merged
merged 10 commits into from
Feb 25, 2021

Conversation

eiffel777
Copy link
Contributor

@eiffel777 eiffel777 commented Feb 2, 2021

Move to using a custom Ingestor class for setting the start and end time for a host configuration in a cloud resource

Description

The custom Ingestor makes it easier to track when a host for a cloud resource changes the number of vcps or memory it contains and when a host may be taken out of a cloud resource.

Motivation and Context

This changes eliminates bugs in the previous implementation and makes it clearer to understand what is going on.

Tests performed

Tested in docker with fabricated test data and add new component tests

Checklist:

  • The pull request description is suitable for a Changelog entry
  • The milestone is set correctly on the pull request
  • The appropriate labels have been added to the pull request

@eiffel777 eiffel777 added bug Bugfixes Category:Cloud Cloud Realm labels Feb 2, 2021
@eiffel777 eiffel777 added this to the 9.5.0 milestone Feb 2, 2021
@eiffel777 eiffel777 self-assigned this Feb 2, 2021
'hostname' => $srcRecord['hostname'],
'vcpus' => $srcRecord['vcpus'],
'memory_mb' => $srcRecord['memory_mb'],
'start_date_ts' => strtotime($srcRecord['start_date_ts'] . " 00:00:00"),
Copy link
Member

Choose a reason for hiding this comment

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

We conventionally use the _ts postfix to indicate that the data type is a posix timestamp. This looks like it is converting a datetime to a timestamp in place. Is this intended?

Copy link
Contributor Author

@eiffel777 eiffel777 Feb 2, 2021

Choose a reason for hiding this comment

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

@jpwhite4 Yeah, it is a datetime being converted to a timestamp. It's just wasn't named descriptively. I've changed the name to denote it is a date

@eiffel777
Copy link
Contributor Author

@jpwhite4 @ryanrath This is updated and ready to be reviewed

@jpwhite4
Copy link
Member

jpwhite4 commented Feb 23, 2021

This code does not generate correct SQL:

#10 {main}","message":"xdmod.aggregate-cloud-resource-specs.CloudResourceSpecsAggregator (ETL\Aggregator\SimpleAggregator): Error processing aggregation period Exception: 'SQLSTATE[42000]: Syntax error or access violation: 1055 'modw_cloud.crs.vcpus' isn't in GROUP BY'"}

In your tests I suggest merging #1469 so that the framework reports the errors.

EDIT - I thought I was looking at your other pull request but had merged this one instead. However, this is a merge conflict with the other one. Which one do you want to merge first?

}

if ($this->_instance_state === null) {
if($srcRecord['vcpus'] == -1 && $srcRecord['memory_mb'] == -1) {
Copy link
Member

Choose a reason for hiding this comment

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

What scenario is handled here? When would you have the first record have vcpus and memory_mb as -1?

@eiffel777 eiffel777 merged commit a9145bb into ubccr:xdmod9.5 Feb 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Bugfixes Category:Cloud Cloud Realm
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants