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

Fix summariser time CpuCount warnings for cloud accounting #156

Merged
merged 4 commits into from
Apr 5, 2018

Conversation

gregcorbett
Copy link
Member

@gregcorbett gregcorbett commented Mar 22, 2018

The warnings are caused by CloudRecords with a NULL CpuCount forming CloudSummaries with a NULL CpuCount, which causes the warning because CpuCount is now part of the CloudSummaries primary key (following a bug fix in 1.6.1).

I have made a change to cloud.py to prevent new CloudRecords being loaded with a NULL CpuCount, opting for zero as the replacement, so CloudRecords without a CpuCount are as separate as possible from those that do.

I also added test cases to try and highlight what was changing and to show that everything still seems to work.

Manual Testing

Tested by loading the below message repeatedly into a Cloud database (10.1.20-MariaDB) on a SL7 VM

BenchmarkType: HEPSPEC06
Status: completed
SiteName: Test Site
MachineName: Test Machine
ImageId: Test Image ID
LocalUserId: Test Local User ID
FQAN: NULL
LocalGroupId: Test Local Group ID
VMUUID: datetime.now()
CloudType: caso/0.3.4 (OpenStack)
GlobalUserName: Test User
CloudComputeService: Test Service

under 1.6.1, when loading that message we pass NULL to CpuCount, rather than relying on implicit type correction that's not supported out of the box in newer mysql/mariadb. Woo!

+----------------------------+-----------+----------+
| VMUUID                     | SiteName  | CpuCount |
+----------------------------+-----------+----------+
| 2018-03-22 16:24:01.535679 | Test Site |     NULL |
+----------------------------+-----------+----------+

This causes the summariser warnings, because CpuCount is in the primary key and hence NOT NULL in the CloudSummaries table.

Query OK, 1 row affected, 3 warnings (0.02 sec)

MariaDB [cloud_dev]> show warnings;
+---------+------+----------------------------------+
| Level   | Code | Message                          |
+---------+------+----------------------------------+
| Warning | 1048 | Column 'Month' cannot be null    |
| Warning | 1048 | Column 'Year' cannot be null     |
| Warning | 1048 | Column 'CpuCount' cannot be null |
+---------+------+----------------------------------+

We get away with it because of the implict type correction of NULL to 0 that's not supported out of the box in newer mysql/mariadb. Boo!

+---------------------+-----------+----------+
| UpdateTime          | SiteName  | CpuCount |
+---------------------+-----------+----------+
| 2018-03-22 16:25:54 | Test Site |        0 |
+---------------------+-----------+----------+

To fix existing data, applying the update script

mysql -u root cloud_dev < scripts/update-1.6.1-next_version.sql
+----------------------------+-----------+----------+
| VMUUID                     | SiteName  | CpuCount |
+----------------------------+-----------+----------+
| 2018-03-22 16:24:01.535679 | Test Site |        0 |
+----------------------------+-----------+----------+

MariaDB [cloud_dev]> Call SummariseVMs();
Query OK, 2 rows affected, 2 warnings (0.02 sec)

MariaDB [cloud_dev]> show warnings;
+---------+------+-------------------------------+
| Level   | Code | Message                       |
+---------+------+-------------------------------+
| Warning | 1048 | Column 'Month' cannot be null |
| Warning | 1048 | Column 'Year' cannot be null  |
+---------+------+-------------------------------+

and no stale summaries :)

+---------------------+-----------+----------+
| UpdateTime          | SiteName  | CpuCount |
+---------------------+-----------+----------+
| 2018-03-22 16:28:03 | Test Site |        0 |
+---------------------+-----------+----------+

The changes to the cloud.py mean newly loaded records without a CpuCount get a CpuCount of 0.

+----------------------------+-----------+----------+
| VMUUID                     | SiteName  | CpuCount |
+----------------------------+-----------+----------+
| 2018-03-22 16:24:01.535679 | Test Site |        0 |
| 2018-03-22 16:30:20.062425 | Test Site |        0 |
+----------------------------+-----------+----------+

Copy link
Member

@tofu-rocketry tofu-rocketry left a comment

Choose a reason for hiding this comment

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

Looks good. Just a couple of things in the comments.


-- This section will:
-- - Change records with a NULL CpuCount so that they have a CpuCount of 0,
-- to prevent summariser time problems.
Copy link
Member

Choose a reason for hiding this comment

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

Little ambiguous. You mean problems at summarising time, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, change made

@@ -290,5 +295,24 @@ def test_last_update(self):
CloudType: Cloud Technology 2
'''

# A Cloud V0.4 Record, but missing a lot of fields.
# We need to check we can this record as we receive
Copy link
Member

Choose a reason for hiding this comment

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

Missing "load"?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, change made

@gregcorbett
Copy link
Member Author

Changes made

- the test captures what happens as of 1.6.1 and
  helps convince us we aren't breaking anything
  when making any future chnages to default
  CloudRecord values.
- Helps convince us we aren't breaking anything
  when making any future changes to default
  CloudRecord values
- when it is ommited, NULL is written as the CpuCount in the CloudRecord
  table, which causes problems at summariser time as
  NULL CpuCounts aren't allowed in the CloudSummaries table
- Cleans up any existing cloud data with a NULL CpuCount,
  which causes warnings when we try and insert cloud
  summarises with NULL CpuCounts into the cloud summary table
@tofu-rocketry
Copy link
Member

I've split the comment fix commit, squashed each bit into the relevant commit, and rebased.

@tofu-rocketry tofu-rocketry merged commit d25dbca into apel:dev Apr 5, 2018
@gregcorbett gregcorbett deleted the cpu_count_summariser_fix branch April 12, 2018 09:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

None yet

2 participants