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

JPERF-790: Fix Jira Standalone testing for pre version 8.9.0 #123

Merged
merged 1 commit into from
Jan 20, 2022

Conversation

pczuj
Copy link
Contributor

@pczuj pczuj commented Jan 17, 2022

No description provided.

@pczuj pczuj requested a review from a team as a code owner January 17, 2022 17:59
@@ -249,6 +255,10 @@ class StandaloneFormula private constructor(
val databaseDataLocation = setupDatabase.get()
val node = time("start") { provisionedNode.start(emptyList()) }

if (!selfAccess.get()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

For future changes we might think of timeouts

Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed, there should be a timeout for every Future.get call in all of JPT, to avoid hanging.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will not implement this suggestion i this PR - I believe we should create an issue about it

mgrzaslewicz
mgrzaslewicz previously approved these changes Jan 18, 2022
@pczuj pczuj requested a review from a team January 18, 2022 13:54
dagguh
dagguh previously approved these changes Jan 18, 2022
@@ -224,6 +224,12 @@ class StandaloneFormula private constructor(
)
)

// For Jira pre 8.9.0 if the instance has no access to its own HTTP the dashboard view may freeze after log in.
Copy link
Contributor

Choose a reason for hiding this comment

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

We can update DataCenterFormulaIT to cover 8.9.0

Copy link
Contributor Author

@pczuj pczuj Jan 18, 2022

Choose a reason for hiding this comment

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

If we are fine with adding this as another scenario (additional test time) then sure I can add it. If you have an idea how to add it and not increase test time then I'm also willing to implement it.

I'd suggest to add test for Jira 8.0.0 (8.9.0 is actually the first version not being affected) as this was the version that was failing in our internal tests. Alternatively we could test 8.5 as this was the LTS and would cover for another version than the one that we already have in tests of iJPT (not sure if this is correct direction of thinking)

Please let me know if adding another test is fine. Or would you change the current Standalone test instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

It already covers 8.0.0. We could switch that to 8.9.0, unless the current coverage is important for some reason.

Copy link
Contributor

Choose a reason for hiding this comment

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

how to add it and not increase test time

The test code is concurrent already (if we need to preserve the old variation).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, an important fact: We are in StandaloneFormula, so the test would be StandaloneFormulaIT and it doens't cover 8.0.0: https://github.com/atlassian/aws-infrastructure/blob/master/src/test/kotlin/com/atlassian/performance/tools/awsinfrastructure/api/jira/StandaloneFormulaIT.kt

It has JSM 3.9.6 test only, which seems to not cover the problematic case (not sure why)

Copy link
Contributor Author

@pczuj pczuj Jan 18, 2022

Choose a reason for hiding this comment

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

Ok, I will add the JSW 8.0.0 Standalone test. Hopefully it will reproduce the problem and will show how the problem is fixed then.

  • Add 8.0.0 Standalone IT

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now that I started to write the test I'm realising that it's not so simple from the abstraction point of view.

If it's part of StandaloneFormulaIT should this test use VU? We didn't do that so far in DataCenterFormulaIT nor in StandaloneFormulaIT and I don't know about simpler way to log in into Jira inside JPT code. Log in is the operation that fails on Jira 8.0.0.

I currently don't see how to do this correctly in the current shape of the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Speaking of the shape of the code and the IT: We wouldn't need ITs in this repository if the lib would promise behavior (that something will be done) rather than promise outcome (that something will work). This fix would not be needed if the contract of shaping permissions between instances would be given outside of the lib, however because we promise things to work (Jira application precisely) we are forced to manage the firewall here.

I think that the performance test should be the IT for the "should work" part and this could be achieved by opening every single behavior of this lib into the API. I'm aware that it could enforce us to break API more, however this is not really unavoidable if we'd follow some other practices like versioning inside class's name.

Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps infra could use VUs to test whether the provisioned Jira is workable. We'd need to resolve circular dependency though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm still not sure how this test would be implemented. I'm planning to skip it.

The access is required based on our documentation, so I'd say adding it doesn't require us to prove it's needed with test.

For Jira pre 8.9.0 if the instance has no access to its own HTTP the dashboard view may freeze (in our case it was after log in, however it may be related to dataset config).
This access to self is described as required in the setup documentation https://confluence.atlassian.com/jirakb/configure-linux-firewall-for-jira-applications-741933610.html and it was missed in implementation of aws-infrastructure 2.24.0

> 4 - Allowing connections to JIRA from itself (to ensure you don't run into problems with [gadget titles showing as __MSG_gadget](https://confluence.atlassian.com/jirakb/fix-gadget-titles-showing-as-__msg_gadget-in-jira-server-813697086.html))
> ```iptables -t nat -I OUTPUT -p tcp -o lo --dport 80 -j REDIRECT --to-ports 8080```
@pczuj pczuj dismissed stale reviews from dagguh and mgrzaslewicz via 0ae6ff1 January 20, 2022 13:01
@pczuj pczuj force-pushed the JPERF-790-fix-standalone-before-jira-8.9.0 branch from caa4af6 to 0ae6ff1 Compare January 20, 2022 13:01
@pczuj
Copy link
Contributor Author

pczuj commented Jan 20, 2022

I changed the commit a bit. I noticed that the access to self is described in our docs: https://confluence.atlassian.com/jirakb/configure-linux-firewall-for-jira-applications-741933610.html

4 - Allowing connections to JIRA from itself (to ensure you don't run into problems with gadget titles showing as __MSG_gadget)
iptables -t nat -I OUTPUT -p tcp -o lo --dport 80 -j REDIRECT --to-ports 8080

@dagguh
Copy link
Contributor

dagguh commented Jan 20, 2022

This could become a new behavioral contract of the formula.

@pczuj
Copy link
Contributor Author

pczuj commented Jan 20, 2022

The visibility between instances used internally in JiraFormula implementations is a problem. For instance last week one team contacted me with a problem that their tests stopped working, because they were configuring LDAP on sharedhome instance and Jira was supposed to have access to it's port. This exact functionality with sharedhome instance shouldn't be supported, because we don't have API for overwriting sharedhome configuration over ssh (they are hacking it by parsing detailed.log), however we do have API for configuring Jira and database instances. Right now only mysql port is open between those two. If anyone would have Database implementation that would require other port(s) then it wouldn't work.

I think we need to either be less strict and allow any communication between those instances or we should allow API consumers to define those accesses. I don't like both solutions i the current form of JiraFormula implementations.

Anyway I'm gonna followup on this in another changeset.

@dagguh
Copy link
Contributor

dagguh commented Jan 20, 2022

Hmm, perhaps resurrecting my hook PR would enable that fine-grained control.

@dagguh
Copy link
Contributor

dagguh commented Jan 20, 2022

PS. behavioral contract only requires a Javadoc promise. The test coverage is nice-to-have, but not technically necessary.

@pczuj pczuj merged commit 9e18c57 into master Jan 20, 2022
@pczuj pczuj deleted the JPERF-790-fix-standalone-before-jira-8.9.0 branch January 20, 2022 13:21
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.

3 participants