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

Issue/jperf 273 transplant ssh jira nodes #7

Draft
wants to merge 28 commits into
base: master
Choose a base branch
from

Conversation

dagguh
Copy link
Member

@dagguh dagguh commented Apr 1, 2019

Desired effects:

  • both aws-infrastructure main and virtual-users tests can reuse the same SSH Jira API
  • aws-infrastructure being able to optimize provisioning, e.g. :
    • preemptively install all DC nodes and then start them in sequence without additional delays
    • parallelize DB, shared home and DC node setup (only blocking before the Jira start)
  • flexibility to inject arbitrary logic into specific points during SSH Jira lifetime (profilers, upgrade task triggering and polling, JVM config, DB config, system metrics)
  • composition of a custom set of arbitrary hooks (e.g. do not run poll upgrade tasks if I don't want to)
  • smart report production and consumption, e.g. don't transfer analytics logs if I'm not consuming them, also don't install vmstat if I'm not going to run it)
  • thread-safety - let hooks be immutable
  • rich library of battle-tested hooks
  • ability to add PostgreSQL support within infrastructure (without aws-infrastructure, we can reuse the same port), or even without any changes to infrastructure at all (custom hook)
  • aws-infrastructure can add its own hook for the Jira plugins transported via S3

JPERF-273 would be resolved with the first bullet point, but if we simply move the aws-infrastructure StandaloneStoppedNode, etc., then we'll have to live with the bad API for a long time. So I'm trying to make new, designed, more flexible API, which should stay useful for a longer time.

@sebapawlak
Copy link
Contributor

Could you explain what is the general purpose/value proposition of this PR?

@dagguh
Copy link
Member Author

dagguh commented Apr 2, 2019

It's a draft, I'll add more context to the commit messages when it's ready (the scope is in flux as I discover new obstacles or opportunities). But the overall goal is to ship https://ecosystem.atlassian.net/browse/JPERF-273

Sed().replace(
connection = ssh,
expression = "(<url>.*(@(//)?|//))" + "([^:/]+)" + "(.*</url>)",
Copy link
Member Author

Choose a reason for hiding this comment

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

It still preserves the old assumption that the dbconfig.xml from Jira home is set up for MySQL already and all we need to do is to switch the IP. We can make it more robust in another PR.
Here's how the assumed dbconfig.xml looks like:

<?xml version="1.0" encoding="UTF-8"?>

<jira-database-config>
  <name>defaultDS</name>
  <delegator-name>default</delegator-name>
  <database-type>mysql</database-type>
  <jdbc-datasource>
    <url>jdbc:mysql://18.184.122.128:3306/jiradb?useUnicode=true&amp;characterEncoding=UTF8&amp;sessionVariables=storage_engine=InnoDB</url>
    <driver-class>com.mysql.jdbc.Driver</driver-class>
    <username>jirauser</username>
    <password>jirauser</password>
    <pool-min-size>20</pool-min-size>
    <pool-max-size>20</pool-max-size>
    <pool-max-wait>30000</pool-max-wait>
    <validation-query>select 1</validation-query>
    <min-evictable-idle-time-millis>60000</min-evictable-idle-time-millis>
    <time-between-eviction-runs-millis>300000</time-between-eviction-runs-millis>
    <pool-max-idle>20</pool-max-idle>
    <pool-remove-abandoned>true</pool-remove-abandoned>
    <pool-remove-abandoned-timeout>300</pool-remove-abandoned-timeout>
    <pool-test-while-idle>true</pool-test-while-idle>
    <validation-query-timeout>3</validation-query-timeout>
  </jdbc-datasource>
</jira-database-config>

Copy link
Contributor

Choose a reason for hiding this comment

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

IMO it's a good moment to stop relying on the assumption.

listOf(
JiraLogs(),
JstatHook()
//,RestUpgrade(config.launchTimeouts, "admin", "admin") TODO requires database to work
Copy link
Member Author

Choose a reason for hiding this comment

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

Can we control the order of things happening before or after the upgrade?

) {
val localSharedHome = sharedHome.localSharedHome
sharedHome.mount(ssh)
val jiraHome = jira.home // TODO what's the difference between localSharedHome and jiraHome? should both be hookable?
Copy link
Contributor

@pczuj pczuj Apr 15, 2019

Choose a reason for hiding this comment

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

flow: JiraNodeFlow
): InstalledJira {
val installation = productDistribution.install(ssh, ".")
val home = jiraHomeSource.download(ssh)
Copy link
Contributor

@pczuj pczuj Apr 15, 2019

Choose a reason for hiding this comment

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

Just a note: I'd remove the knowledge of jirahome being downloaded from JiraHomeSource and make it have #install(ssh) as well. The JiraHomeSource could in the future be implemented as offline jirahome generator.

import com.atlassian.performance.tools.infrastructure.api.jira.flow.JiraNodeFlow
import com.atlassian.performance.tools.ssh.api.SshConnection

class JiraHomeProperty : InstalledJiraHook {
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be part of jirahome installation

import com.atlassian.performance.tools.infrastructure.api.jira.flow.JiraNodeFlow
import com.atlassian.performance.tools.ssh.api.SshConnection

interface JiraInstallation {
Copy link
Contributor

Choose a reason for hiding this comment

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

Funny that this is also indirectly TcpServerHook (logicly), but the TcpServerHook is also InstalledJiraHook, so basically you could install Jira on top of tcp server with Jira, which makes sense.

import com.atlassian.performance.tools.infrastructure.api.jira.flow.JiraNodeFlow
import com.atlassian.performance.tools.ssh.api.SshConnection

class DataCenterHook(
Copy link
Contributor

@pczuj pczuj Apr 15, 2019

Choose a reason for hiding this comment

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

I'd name it differently, like e.g. UpgradeToDataCenter or DataCenterUpgrade or DataCenterInstallation

import com.atlassian.performance.tools.infrastructure.api.jira.flow.report.StaticReport
import com.atlassian.performance.tools.ssh.api.SshConnection

class SystemLog : Report {
Copy link
Contributor

Choose a reason for hiding this comment

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

class SystemLog : Report by StaticReport("/var/log/syslog")

listOf(
JiraLogs(),
JstatHook()
//,RestUpgrade(config.launchTimeouts, "admin", "admin") TODO requires database to work
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you mean credentials? This knowledge from database could be bypassed if the user would always be admin/admin (or any defined in the test config).
Following KB could be the material for another "Hook" that would ensure that: https://confluence.atlassian.com/jira/retrieving-the-jira-administrator-192836.html
We would need to have an sql client that could work with every database Jira supports - possibly some kind of application to reset jira password.

Copy link
Member Author

@dagguh dagguh Dec 10, 2020

Choose a reason for hiding this comment

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

I meant that RestUpgrade has a hidden dependency on DB. All other hooks only depended on what's injected in their constructors or method params. I guess the REST upgrade hook could become an injectable part of the DB hook.
If that happens then the credentials are an internal contract of the hook, so that's all good.

  • try injecting REST upgrades into DB hook

@dagguh
Copy link
Member Author

dagguh commented Apr 17, 2019

FYI I'm doing this on my 20% time, so expect slow progress


interface InstallableDatabase : Database {

fun installInJira(databaseIp: String): Install
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not JDBC URL? Is it enough to set up a database by knowing only the IP address?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because the callers don't know the JDBC URL. aws-infrastructure only generates the IP.
Perhaps this is the same smell as in https://github.com/atlassian/aws-infrastructure/pull/15/files#r273025547 : we're designing low-level API to match the existing caller patterns.
Perhaps it's the aws-infrastructure, who should ask for the fun install(databaseIp: String): Install

Sed().replace(
connection = ssh,
expression = "(<url>.*(@(//)?|//))" + "([^:/]+)" + "(.*</url>)",
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO it's a good moment to stop relying on the assumption.

@@ -26,7 +26,7 @@ class ThreadDump(
*/
fun gather(connection: SshConnection, destination: String) {
val threadDumpName = Instant.now().toEpochMilli()
val command = "${jdk.use()}; jcmd $pid Thread.print > $destination/${threadDumpName}"
val command = "${jdk.use()}; jcmd $pid Thread.print > $destination/$threadDumpName"
Copy link
Contributor

Choose a reason for hiding this comment

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

You could limit the cosmetic changes in the large PR like this one.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, I'll try to make this PR smaller. It's too big to comprehend.

@dagguh dagguh force-pushed the issue/JPERF-273-transplant-ssh-jira-nodes branch 2 times, most recently from e665f02 to d3c7a96 Compare July 5, 2019 18:19
@dagguh dagguh force-pushed the issue/JPERF-273-transplant-ssh-jira-nodes branch from d3c7a96 to e18d03e Compare September 6, 2019 15:09
import com.atlassian.performance.tools.infrastructure.api.jira.flow.install.InstalledJiraHook
import com.atlassian.performance.tools.ssh.api.SshConnection

class DatabaseIpConfig(
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a test to cover this?

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, but right now it's externalized.

We can include this coverage in here via SshUbuntu


class DatabaseIpConfig(
private val databaseIp: String
) : InstalledJiraHook {
Copy link
Contributor

Choose a reason for hiding this comment

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

I remember we were discussing the meaning of this hooks F2F, but now I'm getting back to this pull-request after a while and I feel I don't have a clear picture on the hook system. Do you plan to add Kdocs?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, all API should have KDocs.
Until then, we can IDE and "find usages".

Copy link
Member Author

Choose a reason for hiding this comment

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

  • KDoc for interfaces
  • KDoc for classes

import com.atlassian.performance.tools.ssh.api.SshConnection
import java.time.Duration

class MysqlConnector : InstalledJiraHook {
Copy link
Contributor

Choose a reason for hiding this comment

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

MysqlConnector#run will not run MysqlConnector. Can we consider renaming to MysqlConnectorInstaller (It's ugly but more accurate)? WDYT?

Copy link
Member Author

Choose a reason for hiding this comment

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

Fair enough

dagguh and others added 22 commits January 22, 2021 15:46
Allow reports to be contributed from arbitrary phase of the lifecycle.
It means we can download partial results if the lifecycle fails mid-way.
It's already possible to hook in DB installation from `InstallableDatabase`.
Show the problem with hooking onto the tail of the hook queue during hook iteration.
This also means that if you run a hook, it gets unregistered
Use destructive polling for all JiraNodeFlow hook types.
Add JavaDoc.
Use terminology of "insert" and "call" as per [common terminology].
Repackage hooks.

[common terminology]: https://en.wikipedia.org/wiki/Hooking
@dagguh dagguh force-pushed the issue/JPERF-273-transplant-ssh-jira-nodes branch from caa979a to 2742b75 Compare January 22, 2021 15:57
Comment on lines 13 to 16
val postInstall = preInstall.postInstall
val preStart = postInstall.preStart
val postStart = preStart.postStart
val reports = postStart.reports
Copy link
Member Author

Choose a reason for hiding this comment

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

These are all already available via preInstall. They serve as syntax sugar to avoid long chains.
Before:

hooks.preInstall.postInstall.preStart.postStart.reports.list()

After:

hooks.reports.list()

import com.atlassian.performance.tools.ssh.api.SshConnection
import java.net.URI

class AsyncProfilerHook : PreInstallHook {
Copy link
Member Author

Choose a reason for hiding this comment

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

@mzyromski-atlassian This is one of the more advanced usage of hooks.
It installs a profiler before Jira is installed. Then starts the profiler after Jira starts (using PID provided by hooks). And it registers a flamegraph file for download.

@dagguh dagguh requested a review from a team January 22, 2021 16:05
@dagguh
Copy link
Member Author

dagguh commented Jan 29, 2021

@mzyromski-atlassian this is too big, so I started splitting it.
Here are the chunks:

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.

4 participants