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

bump embedded Weave Net version to 2.1.3 #2975

Merged
merged 1 commit into from
Dec 11, 2017
Merged

bump embedded Weave Net version to 2.1.3 #2975

merged 1 commit into from
Dec 11, 2017

Conversation

rade
Copy link
Member

@rade rade commented Dec 11, 2017

I've tested this while running

  • no Weave Net
  • Weave Net 1.9.0
  • Weave Net 2.1.3

Precursor to addressing #2974.

@rade rade requested a review from bboreham December 11, 2017 11:42
@rade
Copy link
Member Author

rade commented Dec 11, 2017

Weave Net 1.9.0

There's actually a problem here:

<app> WARN: 2017/12/11 14:24:00.073314 Error updating weaveDNS, backing off 20s: Error running weave expose: exit status 4: ""

which occurs because in 2.1, expose is done via a new endpoint on the http API.

That breaks registering of the app with weaveDNS and hence discovery of the app via weaveDNS by probes.

I think it's ok to say to users who want that functionality in Scope 1.7 that they need to run a 2.1 Weave Net. @bboreham what do you reckon?

@rade rade mentioned this pull request Dec 11, 2017
@bboreham
Copy link
Collaborator

Why is it calling expose ? I haven't dug all the way through the chain of dependencies.

@rade
Copy link
Member Author

rade commented Dec 11, 2017

Why is it calling expose?

I was wondering the same. My theory is that it's done in order to get a Weave IP (and register that in WeaveDNS) even though the app is running in the host netns. It was introduced a long time ago in #302.

@bboreham
Copy link
Collaborator

Seems to come from #302, and the code seems to have used DNS before that.

Certainly the code now registers all available IP addresses with WeaveDNS, so it isn't specifically trying to get one on the Weave network.

Looks unnecessary to me.

@rade
Copy link
Member Author

rade commented Dec 11, 2017

Looks unnecessary to me.

It is necessary for probes to able to connect to apps over the weave network.

Note that it checks first whether there's already an exposed address, So this would still work for users of Weave Net prior to version 2.1 provided they run weave expose themselves.

@bboreham
Copy link
Collaborator

It is necessary for probes to able to connect to apps over the weave network.

Do you mean it is aimed at the case where the probe cannot connect to the app except over the Weave network?

AFAICS it registers every address the app is listening on, so which address the probe chooses is unpredictable, so in those cases it would fail much of the time.

@rade
Copy link
Member Author

rade commented Dec 11, 2017

Do you mean it is aimed at the case where the probe cannot connect to the app except over the Weave network?

Yes.

AFAICS it registers every address the app is listening on,

Correct.

so which address the probe chooses is unpredictable, so in those cases it would fail much of the time.

It sends to all of them.

@rade rade merged commit 6f02432 into master Dec 11, 2017
@rade rade deleted the bump-weavenet branch December 25, 2017 10:14
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.

2 participants