HPCC-14535 Fetch all available ESPs and verify ESP is active #9642

rpastrana · 2017-02-28T22:16:45Z

Fetches all available wsfs instances
Verifies given wsfs is accessible

Signed-off-by: Rodrigo Pastrana rodrigo.pastrana@lexisnexis.com

Type of change:

This change is a bug fix (non-breaking change which fixes an issue).
This change is a new feature (non-breaking change which adds functionality).
This change is a breaking change (fix or feature that will cause existing behavior to change).
This change alters the query API (existing queries will have to be recompiled)

Checklist:

Testing:

hpcc-jirabot · 2017-02-28T22:16:53Z

https://track.hpccsystems.com/browse/HPCC-14535
Jira updated

rpastrana · 2017-02-28T22:18:00Z

@wangkx please review

wangkx

Please see my comments.

wangkx · 2017-03-02T15:37:16Z

plugins/fileservices/fileservices.cpp

+{
+    StringBuffer key;
+    CriticalBlock b(espURLcrit);
+    espUrlsLastHeartBeat.setValue(key.set(url).str(), aliveTime);


Looks like that url will not be null. Why not use url directly?

I needed to persist url.

I don't understand this answer. The copy into key does not make any sense to me unless you are trying to translate nullptr to "" (but it's an inefficient way to do that...)

wangkx · 2017-03-02T15:45:51Z

plugins/fileservices/fileservices.cpp

+    server.addServiceUrl(espurl);
+
+    Owned<IClientEchoDateTime> req = server.createEchoDateTimeRequest();
+    Owned<IClientEchoDateTimeResponse> result = server.EchoDateTime(req);


If FileSpray is not running, will EchoDateTime return null or throw exception? How to handle it?

rpastrana · 2017-03-02T19:26:54Z

@wangkx please review.

wangkx · 2017-03-02T20:09:02Z

plugins/fileservices/fileservices.cpp

-    Owned<IClientEchoDateTimeResponse> result = server.EchoDateTime(req);
+    try
+    {
+		Owned<IClientEchoDateTime> req = server.createEchoDateTimeRequest();


@rpastrana Wrong indent.

rpastrana · 2017-03-02T20:31:07Z

@wangkx please review again

wangkx · 2017-03-02T21:01:53Z

@rpastrana looks fine now.

rpastrana · 2017-03-02T21:45:47Z

@richardkchapman please merge

richardkchapman · 2017-03-03T09:53:35Z

plugins/fileservices/fileservices.cpp

+        const char * key =  ((const char *)iter.query().getKey());
+        time_t  lastAlive = *(espUrlsLastHeartBeat.getValue(key));
+
+        if (now.getSimple() - lastAlive < 60 * 60)


You check once an hour? That seems a little infrequent...

Polling intervals are always difficult, the right answer is usually to make it configurable, however based on your overall recommendation, this issue becomes moot.

richardkchapman · 2017-03-03T09:54:58Z

plugins/fileservices/fileservices.cpp

+
+    try
+    {
+        Owned<IClientEchoDateTime> req = server.createEchoDateTimeRequest();


How long does this take to time out if the server is NOT available? And is that a problem?

In practice it's in the low seconds, but the fact we can't control the connect|read, etc timeouts on the generated ESP clients is an issue I'm looking into. I want to expose connection level configuration options.

richardkchapman · 2017-03-03T09:58:37Z

@rpastrana A couple of questions, but also a more general point. I don't really understand why there is a need to be tracking availability / timestamps etc. The bug we are trying to fix is that it doesn't fail over when the server it previously connected to goes away... seems like all we need to do is (a) when first connecting, loop through all listed servers until we find one that responds and (b) repeat that process any time the one we talked to before doesn't respond.

What am I missing?

rpastrana · 2017-03-03T15:24:48Z

@richardkchapman well, the concept was to avoid pinging the ESP service as much as possible while maintaining a "reasonably" spaced heartbeat test.

rpastrana · 2017-03-03T21:37:33Z

@richardkchapman please review.

richardkchapman · 2017-03-06T09:29:20Z

plugins/fileservices/fileservices.cpp

+    {
+        PROGLOG("Could not contact WsFS: '%s'",espurl);
+    }
+    return 0;


should say return false

richardkchapman · 2017-03-06T09:29:39Z

plugins/fileservices/fileservices.cpp

+        if (result->getResult())
+            return true;
+    }
+    catch(...)


Will leak any IException that was thrown...

richardkchapman · 2017-03-06T09:30:36Z

plugins/fileservices/fileservices.cpp

+        const char * currentUrl = availableWsFS.item(index);
+        if (contactWsFS(currentUrl, wu))
+            return currentUrl;
+        //else remove it from the list?


Should NOT remove from the list - if a user has no rights on a given ESP it does not mean that other users don't (and this list is static so shared by all users)

richardkchapman · 2017-03-06T09:33:20Z

plugins/fileservices/fileservices.cpp


+    const char * wsfsurl = getNextAliveWsFSURL(wu);


Bit of a shame to start from beginning of list each time rather than last successful connection (though saving last successful connection per user might be a bit of a pain).

@richardkchapman Would it have to be per user? If they can't connect because of permissions I would think they couldn't connect to any instance of ws_fs.. since I think ws_fs access control is only checked at the feature level?

Therefore I think the list would be the same for all users based on the environment, and if the service isn't accessible because of permissions then the permissions should fail for that user on all instances (unless they aren't all secure... which would be inconsistent and bad).

richardkchapman · 2017-03-06T09:33:59Z

@rpastrana Some comments

rpastrana · 2017-03-06T17:32:59Z

@richardkchapman changes based on latest review squashed in.

rpastrana · 2017-03-06T21:38:39Z

@richardkchapman new Jira based on client request timeout: https://track.hpccsystems.com/browse/HPCC-17178

richardkchapman · 2017-03-15T10:11:47Z

@afishbeck @wangkx please re-review

afishbeck · 2017-03-15T19:32:22Z

plugins/fileservices/fileservices.cpp

+{
+    StringBuffer user, password, token;
+    wu->getSecurityToken(StringBufferAdaptor(token));
+    extractToken(token.str(), wu->queryWuid(), StringBufferAdaptor(user), StringBufferAdaptor(password));


@richardkchapman we have really got to find a better way than storing user credentials in an easily extracted manner within workunits. Especially if we ever start using credentials that are valid for services outside of HPCC functionality.

I think @RussWhitehead is looking into that matter https://track.hpccsystems.com/browse/HPCC-17172

Ok, I commented on that issue. Hopefully we can get rid of these entirely.

Agreed. It seems the ECL Plugin logic shouldn't have to deal with credentials at all, either the environment executing the logic (eclide, etc) should provide the connection logic as well.

I have proposed using Public/Private key files and OpenSSL to manage password encryption. See https://track.hpccsystems.com/browse/HPCC-17172

afishbeck

A few comments.

afishbeck · 2017-03-15T20:05:42Z

plugins/fileservices/fileservices.cpp

                    Owned<IPropertyTreeIterator> iter3 = iter2->query().getElements("AuthenticateFeature");
-                    ForEach(*iter3) {
+                    ForEach(*iter3)


I'm not sure why we are looking at authenticated features. The EspBinding with service="EclWatch" represents a collection of services including ws_fs. Looking for both that and for a standalone EspBinding with service="ws_fs" should be enough. We don't really care how authentication is configured here.

Legacy code, looks to be based on the assumption that looking for eclwatch alone wasn't guaranteed to always contain ws_fs???
Looking for service="ws_fs" will always yield false until the day somebody decides to split wsfs from eclwatch...

EclWatch will always contain ws_fs unless we make a major (very unlikely) change in the future. All services can be run on their own, although as you imply it may not be likely for this one.

The way authorization features ended up being done in the environment.xml is a bit weird, but the authorization isn't the best indicator of the service type, and even worse, another service may decide it needs to check ws_fs permissions even though it isn't the ws_fs service. But again the setup config mechanism is weird.

Agreed on all these points, and your suggestion is the obvious cleanest implementation. But no matter which way we go, we're making assumptions (WsFS will always be part of ecwatch, wsfs auth always associated with wsfs). It does't seem worthwhile to change the legacy implementation, but I'll go ahead a change it.

The above suggestion forces a few more string compares, but saves an entire set of iterations over Auth entries, definitely worthwhile change.

afishbeck · 2017-03-15T20:07:05Z

plugins/fileservices/fileservices.cpp

+                            iter3->query().getProp("@service",wsfsurl.clear())&&
+                            (strcmp(wsfsurl.str(),"ws_fs")==0)){
+                            if (iter2->query().getProp("@protocol",wsfsurl.clear())) {
+                                wsfsurl.append("://");
                                StringBuffer espname;
                                if (iter1->query().getProp("@name",espname)) {
                                    StringBuffer espinst;
                                    if (iter1->query().getProp("Instance[1]/@computer",espinst)) {


We're handling the case of multiple ESP processes where the first is down. But what about the case of a single ESP process with multiple instances where the first INSTANCE is down?

afishbeck · 2017-03-15T20:35:00Z

plugins/fileservices/fileservices.cpp


+    const char * wsfsurl = getNextAliveWsFSURL(wu);


@richardkchapman Would it have to be per user? If they can't connect because of permissions I would think they couldn't connect to any instance of ws_fs.. since I think ws_fs access control is only checked at the feature level?

Therefore I think the list would be the same for all users based on the environment, and if the service isn't accessible because of permissions then the permissions should fail for that user on all instances (unless they aren't all secure... which would be inconsistent and bad).

richardkchapman · 2017-03-16T09:22:51Z

@rpastrana I think this is back with you at present

- Catches exceptions in WSFS method invocation - Creates list of all configured WsFS services - Attempts to contact WsFS before providing that target URL - Cleans up error messages Signed-off-by: Rodrigo Pastrana <rodrigo.pastrana@lexisnexis.com>

- Fetches all available wsfs instances - Verifies given wsfs is accessible Signed-off-by: Rodrigo Pastrana <rodrigo.pastrana@lexisnexis.com>

rpastrana · 2017-03-16T21:10:37Z

plugins/fileservices/fileservices.cpp

+
+        if (env.get())
+        {
+            StringBuffer wsFSUrl;


extracted from the inner loops to avoid re-initialization of the SB objects.

rpastrana · 2017-03-16T21:11:18Z

@afishbeck please review.

HPCCSmoketest · 2017-03-16T21:22:13Z

Automated Smoketest: ✅
Sha: 561edf9
Build: success
ECL Watch: Rebuilding Site

errors	warnings	build time
0	65	94.267 seconds

Install hpccsystems-platform-community_6.3.0-trunk0.el7.x86_64.rpm
HPCC Start: OK

Unit tests result:

Test	total	passed
unittest	93	93
wutoolTest(Dali)	19	19
wutoolTest(Cassandra)	19	19

HPCC Stop: OK
HPCC Uninstall: OK

afishbeck

@rpastrana looks better.

Still wish we could cache the list.

afishbeck · 2017-03-17T00:08:13Z

plugins/fileservices/fileservices.cpp

-                    Owned<IPropertyTreeIterator> iter3 = iter2->query().getElements("AuthenticateFeature");
-                    ForEach(*iter3)
+                    espBindingIter->query().getProp("@service",wsFSUrl.clear());
+                    if (wsFSUrl.length() && (stricmp(wsFSUrl.str(),"EclWatch")==0 || stricmp(wsFSUrl.str(),"ws_fs")==0))


Not a big deal, but I find strieq() easier to read.

Also this is why occasionally a more generic string name is good... if its being used for multiple purposes. This name is no longer self describing.

rpastrana · 2017-03-17T13:20:49Z

@richardkchapman please merge.

wangkx reviewed Mar 2, 2017

View reviewed changes

rpastrana force-pushed the HPCC-14535-fileservicesplugin branch from b927709 to b906db3 Compare March 2, 2017 20:30

richardkchapman reviewed Mar 3, 2017

View reviewed changes

rpastrana force-pushed the HPCC-14535-fileservicesplugin branch 2 times, most recently from 0072524 to 7230017 Compare March 3, 2017 21:36

richardkchapman reviewed Mar 6, 2017

View reviewed changes

rpastrana force-pushed the HPCC-14535-fileservicesplugin branch from 7230017 to 02a9f50 Compare March 6, 2017 17:31

afishbeck reviewed Mar 15, 2017

View reviewed changes

rpastrana added 2 commits March 16, 2017 17:07

HPCC-14535 Fetch all available ESPs and verify ESP is active

561edf9

- Fetches all available wsfs instances - Verifies given wsfs is accessible Signed-off-by: Rodrigo Pastrana <rodrigo.pastrana@lexisnexis.com>

rpastrana force-pushed the HPCC-14535-fileservicesplugin branch from f61d62b to 561edf9 Compare March 16, 2017 21:09

rpastrana commented Mar 16, 2017

View reviewed changes

afishbeck approved these changes Mar 17, 2017

View reviewed changes

richardkchapman merged commit a1b2370 into hpcc-systems:candidate-6.4.0 Mar 17, 2017

HPCC-14535 Fetch all available ESPs and verify ESP is active #9642

HPCC-14535 Fetch all available ESPs and verify ESP is active #9642

Conversation

rpastrana commented Feb 28, 2017 • edited

Type of change:

Checklist:

Testing:

hpcc-jirabot commented Feb 28, 2017

rpastrana commented Feb 28, 2017

wangkx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rpastrana commented Mar 2, 2017

Choose a reason for hiding this comment

rpastrana commented Mar 2, 2017

wangkx commented Mar 2, 2017

rpastrana commented Mar 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richardkchapman commented Mar 3, 2017

rpastrana commented Mar 3, 2017

rpastrana commented Mar 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richardkchapman commented Mar 6, 2017

rpastrana commented Mar 6, 2017

rpastrana commented Mar 6, 2017

richardkchapman commented Mar 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afishbeck left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richardkchapman commented Mar 16, 2017

Choose a reason for hiding this comment

rpastrana commented Mar 16, 2017

HPCCSmoketest commented Mar 16, 2017

afishbeck left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rpastrana commented Mar 17, 2017

rpastrana commented Feb 28, 2017 •

edited