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

Tier1 gvnic in simple instance #203

Conversation

nick-stroud
Copy link
Collaborator

@nick-stroud nick-stroud commented Apr 8, 2022

Submission Checklist

  • Have you installed and run this change against pre-commit? pre-commit install
  • Are all tests passing? make tests
  • If applicable, have you written additional unit tests to cover this
    change?
  • Is unit test coverage still above 80%?
  • Have you updated any application documentation such as READMEs and user
    guides?
  • Have you followed the guidelines in our Contributing document?

@nick-stroud nick-stroud marked this pull request as ready for review April 11, 2022 15:15
@nick-stroud nick-stroud force-pushed the tier1_gvnic_in_simple_instance branch from 0bd5857 to 68735f9 Compare April 11, 2022 21:07
Copy link
Member

@tpdownes tpdownes left a comment

Choose a reason for hiding this comment

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

Address whitespace for sure.

Big picture Q: I see this is doing a simple job of exposing GVNIC + Tier 1 settings for the user in the network configuration. I like that. I believe there are also requirements on the VM image as well as VM family.

I think it is probably best not to address them (allow it to fail at apply) and am wondering if you implicitly agree.

Is there anything else I have to do to make this work? Modify the VPC? I have tried modifying the omnia example to adopt this, and am getting startup-script errors that look like basic networking problems (DNS, no outbound connection, something like that)

resources/compute/simple-instance/main.tf Outdated Show resolved Hide resolved
resources/compute/simple-instance/main.tf Outdated Show resolved Hide resolved
resources/compute/simple-instance/versions.tf Show resolved Hide resolved
resources/compute/simple-instance/versions.tf Show resolved Hide resolved
@tpdownes tpdownes assigned nick-stroud and unassigned tpdownes Apr 11, 2022
@nick-stroud
Copy link
Collaborator Author

I believe there are also requirements on the VM image as well as VM family. I think it is probably best not to address them (allow it to fail at apply) and am wondering if you implicitly agree.

I agree. I have tested that, if machine type is not compatible with tier 1 networking or the OS is not compatible with gVNIC, then apply fails.

@nick-stroud
Copy link
Collaborator Author

I have tried modifying the omnia example to adopt this, and am getting startup-script errors that look like basic networking problems (DNS, no outbound connection, something like that).

I was not able to reproduce. I ran through cloud build and deployed omnia in my local env using bandwidth_tier: tier_1_enabled on the compute instances and they both succeeded. If you believe this is still an issue, can you help me reproduce?

@tpdownes
Copy link
Member

I still can't deploy omnia with Tier 1 enabled. But it is stuck waiting on the startup script to complete and I have no reason to believe it would work with default bandwidth tier. I believe this is ready to go but we should add a targeted integration test with Tier 1.

@tpdownes tpdownes assigned nick-stroud and unassigned tpdownes Apr 12, 2022
@nick-stroud
Copy link
Collaborator Author

I still can't deploy omnia with Tier 1 enabled. But it is stuck waiting on the startup script to complete and I have no reason to believe it would work with default bandwidth tier. I believe this is ready to go but we should add a targeted integration test with Tier 1.

Curious what is going wrong with your omnia example. I deployed omnia again this morning with tier 1 on and got no error. From discussion with Carlos, I have opened a task to add integration testing for this functionality to our backlog.

@nick-stroud nick-stroud merged commit f71d196 into GoogleCloudPlatform:develop Apr 12, 2022
@nick-stroud nick-stroud deleted the tier1_gvnic_in_simple_instance branch April 28, 2022 04:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants