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.

Sign up for GitHub

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

Jump to bottom

Implement pools #79

Merged
merged 36 commits into from
Jun 17, 2019
Merged

Implement pools #79

merged 36 commits into from
Jun 17, 2019

Conversation

notjames
Copy link
Contributor

@notjames notjames commented Jun 7, 2019

Added support for pools. Still really new to Go so constructive assistance appreciated.

notjames added 23 commits June 6, 2019 22:21
@notjames
Implement pools
4e63095
@notjames
Fixup missing Pool method...
3649d17
@notjames
iterative things
48ebc1f
@notjames
Iterative
db354f3
@notjames
Add pool interface.
f93ec71
@notjames
bug fixes - iterative
3c9464f
@notjames
bugfixes still
d3af0b7
@notjames
same same
a171a8d
@notjames
iterative.
fdeca78
@notjames
added pools method
4f4c1c4
@notjames
Fixed pool type in allocatemachineargs
d8d6e2c
@notjames
Fixed Pool method to return nil if non-existend. Fixed schema type in…
95a1faf 
… return of pool to machine.
@notjames
Added Description to pool interface.
5a47fa1
@notjames
temp syntax test
aa6db11
@notjames
test failed. reverted
2d5c75a
@notjames
test failed. reverted
60f7491
@notjames
iterative
0ae1772
@notjames
removed pool from interface_ - probably not needed.
07d78e8
@notjames
missed one Pool method in interface_ remove. Fixed.
858cad8
@notjames
Added test file for pool
c8f20c5
@notjames
Removed .idea after adding to global gitignore.
f2477bb
@notjames
Added pool_test
cb9a9ab
@notjames
Implement pools
c00d3cc 
Add pool test
@jujubot
Copy link
Contributor

jujubot commented Jun 7, 2019

Can one of the admins verify this patch?

1 similar comment
@jujubot
Copy link
Contributor

jujubot commented Jun 7, 2019

Can one of the admins verify this patch?

wallyworld
wallyworld reviewed Jun 11, 2019
View reviewed changes
Copy link
Member

@wallyworld wallyworld left a comment

Choose a reason for hiding this comment

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

This looks great, thank you.
Just a few small things to tweak. Main issue is to add controller tests.

machine.go Outdated Show resolved Hide resolved
machine.go Outdated Show resolved Hide resolved
pool_test.go Outdated Show resolved Hide resolved
pool.go Outdated Show resolved Hide resolved
pool_test.go Outdated Show resolved Hide resolved
controller.go Show resolved Hide resolved
pool.go Outdated Show resolved Hide resolved
@notjames
Copy link
Contributor Author

I will get the rest of these done either tonight or tomorrow. I also found a bug I need to fix where a nil Pool returns nil instead of an empty map. I'll fix that too. Thank you for reviewing!

@notjames
Implement pools
effee2e 
Fixed Pool method to return nil if non-existent. Fixed schema type in return of pool to machine.
Added pool_test
Fixed most of the issues requested in PR#79. There are few left. Committing now and performing an interactive rebase to squash commits.
@notjames
Copy link
Contributor Author

had a production impacting issue occur today so likely won't get back to this until tomorrow. It's at the top of my non-prod list of things to do though.

wallyworld
wallyworld reviewed Jun 14, 2019
View reviewed changes
Copy link
Member

@wallyworld wallyworld left a comment

Choose a reason for hiding this comment

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

Just noticed some new commits. Thank you.
We need a TestPools() similar to TestZones() in controller_test.go
Also, some imports need moving.

machine.go Outdated Show resolved Hide resolved
@notjames
Copy link
Contributor Author

I understand. Those commits were not meant to be checked yet. Sorry. My IDE is responsible for the imports. I'll fix all of that up. I am hoping to have this done today provided I'm able to fix the test bug I exposed. I'm also going to rebase/squash all the commits. When that is done then you'll know I'm finished and ready for full review.

notjames added 3 commits June 14, 2019 13:43
@notjames
Final commit; added tests for controller and interface. Fixed up test…
0d1a8ac 
… data to include pool objects. General bugfixes and removal of test output.
@notjames
parent 8a8cec7
92a6f08 
author Jim Conner <snafu.x@gmail.com> 1559884909 -0700
committer Jim Conner <snafu.x@gmail.com> 1560547939 -0700

Implement pools
Fixed Pool method to return nil if non-existent. Fixed schema type in return of pool to machine.
Added pool_test
Fixed most of the issues requested in PR#79. There are few left.
@notjames
Last commit. All tests implemented and passing for pools.
c3169fb
@notjames
Copy link
Contributor Author

This PR is ready for final review and merge.

wallyworld
wallyworld approved these changes Jun 16, 2019
View reviewed changes
Copy link
Member

@wallyworld wallyworld left a comment

Choose a reason for hiding this comment

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

Thanks! Just one small correction. Then you can merge by typing
$$merge$$
as a comment

interface_test.go Outdated Show resolved Hide resolved
@notjames
Fixed case in resource_uri of interfaceResponse const.
29f73ab 
Signed-off-by: Jim Conner <snafu.x@gmail.com>
@notjames
Copy link
Contributor Author

$$merge$$

@notjames
Copy link
Contributor Author

Hmm, the merge macro seems to not work...?

@mitechie
Copy link

I don't think it'll work for you. The bot wants someone from the team to do it.

$$merge$$

@notjames
Copy link
Contributor Author

Makes sense! Thank you again @mitechie (Rick)

@jujubot jujubot merged commit 65f2e26 into juju:master Jun 17, 2019
@stgraber
Copy link
Contributor

So this broke LXD's MAAS support as any attempt to create a device without the pool property set now gives us:

DBUG[07-27|16:44:52] Failure for task operation: a5c57abb-4458-4f16-9027-c1eda178cbb4: Create container: Create LXC container: device 0: device 2.0 schema check failed: pool: expected map, got nothing

@stgraber
Copy link
Contributor

One of the failing code paths is when CreateDevice is called on a machine, this in turns calls CreateDevice on the controller, neither of those pass a pool (not needed as there is a parent), the response coming from MAAS similarly doesn't include a pool (it's nil) and it being nil causes a validation failure.

@stgraber
Copy link
Contributor

This seems to indicate that CreateDevice on a machine isn't tested or was never tested against an actual MAAS server as I can't see any way this would ever work.

@notjames
Copy link
Contributor Author

Good catch! I don't have time today to look into it, but I'll try and dig in tomorrow.

@stgraber
Copy link
Contributor

@notjames #80 seemed to work for me, but that's only tested for the subset of features that LXD uses

@notjames
Copy link
Contributor Author

@stgraber I will take a look and make sure everything works. We have a few maas implementations so if I can find time, I'll do some more testing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wallyworld wallyworld wallyworld approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@notjames @jujubot @mitechie @stgraber @wallyworld