Skip to content
This repository has been archived by the owner on Sep 30, 2024. It is now read-only.

actualize Vagrant and fix problems on orchestrator-agent api with sqlite backend #445

Merged
merged 18 commits into from
Apr 25, 2018

Conversation

Slach
Copy link
Contributor

@Slach Slach commented Mar 17, 2018

Related issue: #438

Description

This PR actualize Vagrant and fix problems on orchestrator-agent api with sqlite backend
i hope this help use innodbbackupex for data seeding between two mysql nodes

In case this PR introduced Go code changes:

  • contributed code is using same conventions as original code
  • code is formatted via gofmt (please avoid goimports)
  • code is built via ./build.sh
  • code is tested via go test ./go/...

@@ -547,8 +547,8 @@ func AbortSeed(seedId int64) error {
}

// PostCopy will request an agent to invoke post-copy commands
func PostCopy(hostname string) (Agent, error) {
return executeAgentCommand(hostname, "post-copy", nil)
func PostCopy(hostname, sourceHostname string) (Agent, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this change going to break behavior for existing users?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

could you help me figure out howto add optional parameter to martini routes?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@Slach Slach Mar 21, 2018

Choose a reason for hiding this comment

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

@shlomi-noach ok.
i add backwards compatibility on orchestrator-agent here
https://github.com/github/orchestrator-agent/pull/20/files#diff-fbdc8cde28a71961e53c2e83e2483c9aL619

and add some other improvments and tested it with percona-xtrabackup
currenly in my setup i have successfull data seed between nodes over netcat + xtrabackup + xbstream

# scripts, such as exporting variables or whatever. It's yours!
[ -f /etc/default/orchestrator ] && . /etc/default/orchestrator
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please explain why /etc/default/orchestrator? IF anything, I'd add /etc/profile.d/orchestrator which is what I should have done in the first place.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

/etc/default it's a default behavion on debian based distributives

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, i add usage /etc/profile.d/orchestrator and /etc/default/orchestrator

/etc/profile.d/orchestrator will have major priority

@shlomi-noach
Copy link
Collaborator

Thank you! This PR is growing bigger and deals with different problems, so it's becoming difficult for me to review it as a whole. Can you please elaborate on "i add backwards compatibility on orchestrator-agent here" ? Where is the backwards compatibility found?

@@ -164,7 +164,7 @@ func ReadOutdatedAgentsHosts() ([]string, error) {
from
host_agent
where
IFNULL(last_checked < now() - interval ? minute, true)
IFNULL(last_checked < now() - interval ? minute, 1)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this change need for SQLlite compatibility cause SQLite desn't have BOOLEAN literal

func PostCopy(hostname string) (Agent, error) {
return executeAgentCommand(hostname, "post-copy", nil)
func PostCopy(hostname, sourceHostname string) (Agent, error) {
return executeAgentCommand(hostname, fmt.Sprintf("post-copy/?sourceHost=%s", sourceHostname), nil)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

there is backwards compatibility

@Slach
Copy link
Contributor Author

Slach commented Mar 21, 2018

Can you please elaborate on "i add backwards compatibility on orchestrator-agent here" ?
backwards behavior it's here

https://github.com/github/orchestrator-agent/pull/20/files#diff-fbdc8cde28a71961e53c2e83e2483c9aL619
and here
https://github.com/github/orchestrator/pull/445/files#diff-17473f4f3d0788c153babef6ddbba1faR551

@shlomi-noach shlomi-noach self-assigned this Mar 22, 2018
@shlomi-noach
Copy link
Collaborator

I will get back on this early next week.

@Slach
Copy link
Contributor Author

Slach commented Mar 22, 2018

ok. i hope my PR will be merged ;)

Copy link
Collaborator

@shlomi-noach shlomi-noach left a comment

Choose a reason for hiding this comment

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

Thank you for the PR! and I'm sorry for the slow response; I have had some busy days and will have yet more busy days ahead of me.
One critical change, and some questions.

.idea/
*.deb
*.pcap
*.log
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

Vagrantfile Outdated
config.vm.synced_folder '.', '/orchestrator', type: 'rsync',
rsync__auto: true
config.vm.synced_folder '.', '/orchestrator'
#, type: 'rsync', rsync__auto: true
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you please explain this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

rsync__auto want work on my Vagrant environment ;(
when i change some sources on host OS, it's not rsynced into guest maching
ok. i rollback this change

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry, please elaborate. Did you introduce rsync__auto in the first place?

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 try use following variants
1)

 config.vm.synced_folder '.', '/orchestrator', type: 'rsync'

it's require run vagrant reload when i change any file on my host machine
2)

 config.vm.synced_folder '.', '/orchestrator', type: 'rsync',  rsync__auto: true

it not worked, and i don't investigate why
then i just commented this options and default sharing mode over vboxfs works as i expected

@@ -87,7 +87,7 @@ function oinstall() {
cd $mydir
gofmt -s -w go/
rsync -qa ./resources $builddir/orchestrator${prefix}/orchestrator/
rsync -qa ./conf/orchestrator-sample.* $builddir/orchestrator${prefix}/orchestrator/
rsync -qa ./conf/orchestrator-sample*.conf.json $builddir/orchestrator${prefix}/orchestrator/
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

build.sh Outdated
# n CentOD 6 box: we only want the rpms for CentOS6
# Add "-centos6" to the file name.
ls ${TOPDIR:-?}/*.rpm | while read f; do centos_file=$(echo $f | sed -r -e "s/^(.*)-${RELEASE_VERSION}(.*)/\1-centos6-${RELEASE_VERSION}\2/g") ; mv $f $centos_file ; done
if [[ -e /etc/centos-release ]]; then
Copy link
Collaborator

Choose a reason for hiding this comment

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

better: -f

"GraphiteAddr": "",
"GraphitePath": "",
"GraphiteConvertHostnameDotsToUnderscores": true
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

@@ -693,7 +693,7 @@ func executeSeed(seedId int64, targetHostname string, sourceHostname string) err
}

seedFromLogicalVolume := sourceAgent.LogicalVolumes[0]
seedStateId, _ = submitSeedStateEntry(seedId, fmt.Sprintf("Mounting logical volume: %s", seedFromLogicalVolume.Path), "")
seedStateId, _ = submitSeedStateEntry(seedId, fmt.Sprintf("%s Mounting logical volume: %s", sourceHostname, seedFromLogicalVolume.Path), "")
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

seedStateId, _ = submitSeedStateEntry(seedId, fmt.Sprintf("Waiting some time for %s to start listening for incoming data", targetHostname), "")
time.Sleep(2 * time.Second)
seedStateId, _ = submitSeedStateEntry(seedId, fmt.Sprintf("Waiting %d seconds for %s to start listening for incoming data", config.Config.SeedWaitSecondsBeforeSend, targetHostname), "")
time.Sleep(time.Duration(config.Config.SeedWaitSecondsBeforeSend) * time.Second)
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

@@ -138,7 +138,7 @@ var generateSQLBase = []string{
last_checked timestamp NULL DEFAULT NULL,
last_seen timestamp NULL DEFAULT NULL,
mysql_port smallint(5) unsigned DEFAULT NULL,
count_mysql_snapshots smallint(5) unsigned NOT NULL,
count_mysql_snapshots smallint(5) unsigned NOT NULL DEFAULT 0,
Copy link
Collaborator

@shlomi-noach shlomi-noach Mar 29, 2018

Choose a reason for hiding this comment

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

🚫 Must change: existing SQL must never ever change. The most we can do is add a modification in generate_patches.go. However, modifying a column is not supported by sqlite so this change must unfortunately go away.

The value is implicitly 0 anyway, what is the concern?

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. ok. sorry i will rollback this change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes concern is
count_mysql_snapshots not used when try insert into host_agent table
and sql query is failed when agent submitting

# n CentOD 6 box: we only want the rpms for CentOS6
# Add "-centos6" to the file name.
ls ${TOPDIR:-?}/*.rpm | while read f; do centos_file=$(echo $f | sed -r -e "s/^(.*)-${RELEASE_VERSION}(.*)/\1-centos6-${RELEASE_VERSION}\2/g") ; mv $f $centos_file ; done
fi
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

/usr/sbin/service orchestrator start

fi

echo '* * * * * root /usr/bin/orchestrator -c discover -i db1' > /etc/cron.d/orchestrator-discovery

# Discover instances
/usr/bin/orchestrator -c discover -i localhost
/usr/bin/orchestrator --verbose --debug --stack -c redeploy-internal-db
Copy link
Collaborator

Choose a reason for hiding this comment

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

What happened that the above became necessary? Current logic should make it safe to run orchestrator without specifying redeploy-internal-db.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ohh, sorry, i multiple run vagrant up and add redeploy-internal-db to try fix errors when using sqlite backend
i revert this changes right now

@Slach
Copy link
Contributor Author

Slach commented Apr 13, 2018

@shlomi-noach would this PR ready for merge? i think i resolve any recomendation

@shlomi-noach shlomi-noach merged commit b3cb488 into openark:master Apr 25, 2018
@Slach
Copy link
Contributor Author

Slach commented Apr 25, 2018

thanks!

@shlomi-noach
Copy link
Collaborator

Thank you!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants