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

calico-ipam-upgrade Implementation #683

Merged
merged 1 commit into from Feb 23, 2019

Conversation

Projects
None yet
2 participants
@briansan
Copy link
Member

briansan commented Feb 14, 2019

Description

  • adds a new binary calico-ipam-upgrade for migrating the data necessary to upgrade KDD-backed calico installations from using host-local IPAM to Calico IPAM

Todos

  • Tests
  • Documentation
  • Release note
Show resolved Hide resolved cmd/calico-ipam-upgrade/main.go Outdated
Show resolved Hide resolved cmd/calico-ipam-upgrade/main.go Outdated
Show resolved Hide resolved cmd/calico-ipam-upgrade/main.go Outdated
Show resolved Hide resolved cmd/calico-ipam-upgrade/main.go Outdated
Show resolved Hide resolved cmd/calico-ipam-upgrade/main.go Outdated
Show resolved Hide resolved cmd/calico-ipam-upgrade/main.go Outdated
Show resolved Hide resolved cmd/calico-ipam-upgrade/main.go Outdated
Show resolved Hide resolved cmd/calico-ipam-upgrade/main.go Outdated
Show resolved Hide resolved cmd/calico-ipam-upgrade/main.go Outdated
Show resolved Hide resolved cmd/calico-ipam-upgrade/main.go Outdated

@briansan briansan changed the title initial commit of calico-ipam-upgrade code calico-ipam-upgrade Implementation Feb 21, 2019

Show resolved Hide resolved cmd/calico-ipam-upgrade/main.go Outdated
Show resolved Hide resolved cmd/calico-ipam-upgrade/main.go Outdated
Show resolved Hide resolved cmd/calico-ipam-upgrade/main.go Outdated
Show resolved Hide resolved cmd/calico-ipam-upgrade/main.go Outdated
Show resolved Hide resolved cmd/calico-ipam-upgrade/main.go Outdated
Show resolved Hide resolved cmd/calico-ipam-upgrade/main.go Outdated
Show resolved Hide resolved cmd/calico-ipam-upgrade/main.go Outdated
handleID := fmt.Sprintf("%s.%s", netName, containerID)
logrus.WithFields(logrus.Fields{

This comment has been minimized.

@caseydavenport

caseydavenport Feb 22, 2019

Member

I think we do actually want to keep this log as it is - it's really useful to have all this information in the CNI log output.

This comment has been minimized.

@briansan

briansan Feb 22, 2019

Author Member

No problem reverting the change, but isn't it sufficient to just log out the handleID since it's constructed using the netName and containerID?

This comment has been minimized.

@caseydavenport

caseydavenport Feb 22, 2019

Member

I think the main thing is we want a log which includes the WorkloadID as well so that we can tie the two together.

Show resolved Hide resolved cmd/calico-ipam-upgrade/main.go Outdated
Show resolved Hide resolved cmd/calico-ipam-upgrade/main.go Outdated
Show resolved Hide resolved pkg/upgrade/migrate.go Outdated
}
logCtxt.WithField("handleID", handleID).Info("file contains handle ID")

alreadyExists := false

This comment has been minimized.

@caseydavenport

caseydavenport Feb 23, 2019

Member

IIUC, I think a more robust way to do this check would be to try to assign the IP address and if you get an AlreadyExists error, you continue.

It means you could remove a bunch of code by getting rid of the IPsByHandle call above altogether, as well as this cross-correlation code to check if it exists. TBH I don't really understand what this loop is meant to check, so I could definitely be wrong.


// Delete the host-local IPAM data directory
log.Info("removing host-local IPAM data directory...")
if err = os.Remove(ipAllocPath); err != nil {

This comment has been minimized.

@caseydavenport

caseydavenport Feb 23, 2019

Member

Will this succeed without us deleting the lock file? I think we need to release the lock and delete the file right before this.

if err = hostLocal.Unlock(); err != nil {
log.WithError(err).Error("failed to release lock on host local backend")
}
log.Info("successfully released lock on host-local backend!")

This comment has been minimized.

@caseydavenport

caseydavenport Feb 23, 2019

Member

This log is going to be emitted even if we fail to release the lock.

@briansan briansan force-pushed the briansan:ipam-upgrade branch from 89c4240 to d158c02 Feb 23, 2019

@caseydavenport caseydavenport merged commit 80baf9f into projectcalico:master Feb 23, 2019

2 checks passed

license/cla Contributor License Agreement is signed.
Details
semaphoreci The build passed on Semaphore.
Details

@briansan briansan deleted the briansan:ipam-upgrade branch Feb 24, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.