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

[#178937872] Adds TCP routing tests to apps on isolation segments #474

Merged

Conversation

geofffranks
Copy link
Contributor

@geofffranks geofffranks commented Aug 11, 2021

What is this change about?

Adds tests to ensure that TCP routed apps work on isolation segments (but not for routed isolation-segments)

Please provide contextual information.

https://www.pivotaltracker.com/story/show/178937872

What version of cf-deployment have you run this cf-acceptance-test change against?

https://github.com/geofffranks/cf-deployment/tree/tcp-isolation / https://github.com/cloudfoundry/cf-deployment

Please check all that apply for this PR:

  • introduces a new test --- Are you sure everyone should be running this test?
  • changes an existing test
  • requires an update to a CATs integration-config

Only runs the new test when both tcp_routing + isolation_segment suites are included

Did you update the README as appropriate for this change?

  • YES
  • N/A

If you are introducing a new acceptance test, what is your rationale for including it CATs rather than your own acceptance test suite?

This test is only enabled when both tcp_routing and isolation_segments are included, and seems the best place to test a CloudFoundry environment using both of these features.

How should this change be described in cf-acceptance-tests release notes?

Adds tests to validate that TCP routed applications function when deployed on a shared-routing isolation segment.

How many more (or fewer) seconds of runtime will this change introduce to CATs?

~300 seconds (3 tests, 2 app pushes) when both TCP routing + isolation segments are included in the test.

What is the level of urgency for publishing this change?

  • Urgent - unblocks current or future work
  • Slightly Less than Urgent

Tag your pair, your PM, and/or team!

It's helpful to tag a few other folks on your team or your team alias in case we need to follow up later.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Aug 11, 2021

CLA Signed

The committers are authorized under a signed CLA.

@Birdrock
Copy link
Member

lgtm - requested review from @acosta11 for additional eyes.

Copy link
Member

@acosta11 acosta11 left a comment

Choose a reason for hiding this comment

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

lgtm as well

@acosta11 acosta11 merged commit b835be2 into cloudfoundry:develop Aug 19, 2021
@a-b
Copy link
Member

a-b commented Sep 8, 2021

@a-b
Copy link
Member

a-b commented Sep 8, 2021

This issue blocks the CF CLI v8 release. If we don't hear from you soon we will have to temporarily revert this PR to let you fix it without pressure.

@davewalter
Copy link
Member

Thanks for reporting your issue @a-b. I can't see the output from either of the CI builds you linked, but this change actually broke our CATs as well. Could you please compare the failures in our CI to yours and let us know if they are the same?

@geofffranks, I am going to revert the change from this PR as I need to merge #473 and fix an issue with another test. I would be happy to revisit this once the other changes are in and validated.

@geofffranks
Copy link
Contributor Author

@a-b I can't see the output either, but looking at https://github.com/cloudfoundry/cli-ci/blob/master/ci/cli-v8/tasks/cats-config.yml, my guess would be that you might not have DNS that points tcp.<apps-domain> to loadbalancers fronting the TCP router on the environment running CATs.

@davewalter I'm not sure how this would have broken https://release-integration.ci.cf-app.com/teams/main/pipelines/cf-deployment/jobs/upgrade-cats/builds/3204. The errors there looks like issues with the CLI being unable to ssh into containers, or set environment variables across multiple test suites. This change only affects the isolation-segment + tcp routing suites.

@davewalter
Copy link
Member

@davewalter I'm not sure how this would have broken https://release-integration.ci.cf-app.com/teams/main/pipelines/cf-deployment/jobs/upgrade-cats/builds/3204. The errors there looks like issues with the CLI being unable to ssh into containers, or set environment variables across multiple test suites. This change only affects the isolation-segment + tcp routing suites.

I'm not sure either. The thing that really confused me was the nature of the failures. For example:

[2021-08-23 16:52:47.96 (UTC)]> cf push CATS-9-APP-9ffb25b2f4ff5e9d -m 256M -p assets/php
Pushing app CATS-9-APP-9ffb25b2f4ff5e9d to org CATS-9-ORG-8eef42608244b4da / space CATS-9-SPACE-f17a0406d9e402e9 as CATS-9-USER-a0aa39aaa4518a53...
FAILED
Space not found

I ran the job 5 times and saw the same (or similar) failures each time. As soon as I pinned the version of CATs back to the previous commit, they passed first time.

I will be happy to dig into it further once #473 is merged and validated.

@geofffranks
Copy link
Contributor Author

@a-b paired with us today to show us the errors in the cf-cli pipeline. They're all over the board as well. I'm wondering if there's something with cleanup interplay or bad things with scope related to the private methods i moved to be helpers that broke things.

Either way there are environmental concerns to getting the tests running, and rather than try to have everyone else fix their environments and set everything up properly, I'm wondering if we should just abandon this test in cats and move it to something like rats, adding isolation segment support there.

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.

None yet

5 participants